Skip to content

ci/treefmt: add markdown-code-runner#427460

Merged
wolfgangwalther merged 3 commits into
NixOS:masterfrom
wolfgangwalther:ci-treefmt-mdcr
Aug 5, 2025
Merged

ci/treefmt: add markdown-code-runner#427460
wolfgangwalther merged 3 commits into
NixOS:masterfrom
wolfgangwalther:ci-treefmt-mdcr

Conversation

@wolfgangwalther

@wolfgangwalther wolfgangwalther commented Jul 22, 2025

Copy link
Copy Markdown
Contributor

This was run as a test in doc/tests/check-nix-code-blocks.nix before, but its DX can be improved: By including it in treefmt we get better error reporting and auto-fixing, as well as running it on all markdown files (including READMEs etc.) for free.

This will need an update of the pin, which needs #427437.

Things done


Add a 👍 reaction to pull requests you find important.

@wolfgangwalther wolfgangwalther requested a review from drupol July 22, 2025 14:23
@nixpkgs-ci nixpkgs-ci Bot added 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog This PR adds or changes release notes 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: tcl Dynamic, multi-paradigm programming language 6.topic: continuous integration Affects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions 8.has: documentation This PR adds or changes documentation 6.topic: policy discussion Discuss policies to work in and around Nixpkgs backport release-25.05 labels Jul 22, 2025
@wolfgangwalther wolfgangwalther requested review from a team and fricklerhandwerk July 22, 2025 14:34
@wolfgangwalther wolfgangwalther mentioned this pull request Jul 22, 2025
1 task
Comment thread ci/default.nix Outdated
@drupol

drupol commented Jul 22, 2025

Copy link
Copy Markdown
Contributor

You can now enjoy a tag for markdown-core-runner: https://github.com/drupol/markdown-code-runner/releases/tag/0.2.0

And the PR that goes with it: #427516

Comment thread ci/default.nix Outdated
Comment thread ci/default.nix Outdated
Comment thread ci/default.nix Outdated
@wolfgangwalther

Copy link
Copy Markdown
Contributor Author

Converted to draft because of the DROP BEFORE MERGE commit in the middle. This pulls in markdown-code-runner from the current branch (not the pin), so that I could already do the formatting.

Formatting is done in 3 steps:

  • First I fix all syntax errors in nix code blocks in the nixos manual.
  • Then I run treefmt->mdcr->nixfmt.
  • Then I fix some comments that were imho misplaced due to the auto formatting.

The first commit looks like I would do it, once the new markdown-code-runner is available in the pinned nixpkgs.

Ready for review, but not for merge.

@nixpkgs-ci nixpkgs-ci Bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. labels Jul 22, 2025
@drupol

drupol commented Jul 22, 2025

Copy link
Copy Markdown
Contributor

To avoid too much work for you in case of conflict(s), I think the commit where you reformat everything should be the last of the list, so that you can easily drop it and recreate it in case of conflict(s).

@MattSturgeon

MattSturgeon commented Jul 22, 2025

Copy link
Copy Markdown
Contributor

Then I fix some comments that were imho misplaced due to the auto formatting.

Just to comment on this with my formatting team hat on:

Trailing comments are moved to their own line in some scenarios (iirc mostly just when they go over the line length), however nixfmt has no way to know if the comment should be moved to before or after the line it was previously on.

In the common example of a comment trailing a binding, it probably is more likely that the comment relates to the variable binding, but in other scenarios that may not be the case.

In most scenarios we'd anticipate the author noticing the auto-formatting not matching their desired layout and manually tweaking it, especially if they have configured treefmt-on-save in their editor.

Perhaps it's worth discussing this in an issue on the nixfmt repo though, so that we can go through some concrete examples and/or get insight from other formatting team members more familiar with the actual implementation. Maybe there is a strong argument for assuming the comment should usually go above instead of below?

@MattSturgeon

This comment was marked as off-topic.

@wolfgangwalther

Copy link
Copy Markdown
Contributor Author

Maybe there is a strong argument for assuming the comment should usually go above instead of below?

I'd argue:

  • A comment at the end of the line is always about that line.
  • Comments on separate lines should usually go before the thing that they concern.

There are probably edge-cases where just moving the comment to the next line is the right thing... but my guess is that the majority of cases is better handled by moving them to the previous line instead.

Comment thread ci/default.nix Outdated
@nixpkgs-ci nixpkgs-ci Bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Aug 5, 2025
Comment thread nixos/doc/manual/development/assertions.section.md Outdated
Comment thread nixos/doc/manual/development/importing-modules.section.md Outdated
Comment thread ci/default.nix Outdated
Comment thread ci/default.nix Outdated
@philiptaron

Copy link
Copy Markdown
Contributor

(Note that the ✅ plus comments means that following the suggestion is extremely optional.)

This was run as a test in `doc/tests/check-nix-code-blocks.nix` before,
but its DX can be improved: By including it in `treefmt` we get better
error reporting and auto-fixing, as well as running it on *all* markdown
files (including READMEs etc.) for free.

@philiptaron philiptaron left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀🌲🔢

Let's roll.

@nixpkgs-ci nixpkgs-ci Bot added 12.approvals: 2 This PR was reviewed and approved by two persons. and removed 12.approvals: 1 This PR was reviewed and approved by one person. labels Aug 5, 2025
@MattSturgeon

Copy link
Copy Markdown
Contributor

Strange CI failure here.

https://github.com/NixOS/nixpkgs/actions/runs/16754575116/job/47433571147?pr=427460

Unhandled error: Error: Could not find a push.yml workflow run for be2b8ea.

That commit is part of #430904, and CI never ran against that specific commit because it was pushed together with a later commit.

I'm not sure why this PR's eval is looking for a workflow run for an entirely unrelated commit, though?

@philiptaron

Copy link
Copy Markdown
Contributor

We just were impacted by https://www.githubstatus.com/incidents/6swp0zf7lk8h @MattSturgeon

@MattSturgeon

Copy link
Copy Markdown
Contributor

Strange failure mode for our get-merge-commit to return a random unrelated target commit... Oh well 😁

Do you have permission to restart the workflow run?

@philiptaron

Copy link
Copy Markdown
Contributor

I do have permissions, but since the failures were so widespread, I expect a rebase would be best to reset it all.

@wolfgangwalther

Copy link
Copy Markdown
Contributor Author

We can re-trigger CI without a rebase by close+open.

@wolfgangwalther wolfgangwalther merged commit be57485 into NixOS:master Aug 5, 2025
40 of 45 checks passed
@wolfgangwalther wolfgangwalther deleted the ci-treefmt-mdcr branch August 5, 2025 18:04
@nixpkgs-ci

This comment was marked as resolved.

@wolfgangwalther wolfgangwalther added the 8.has: port to stable This PR already has a backport to the stable release. label Aug 5, 2025
@nixos-discourse

Copy link
Copy Markdown

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/formatting-team-meeting-2025-08-05/67630/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: continuous integration Affects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: policy discussion Discuss policies to work in and around Nixpkgs 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: port to stable This PR already has a backport to the stable release. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.approvals: 2 This PR was reviewed and approved by two persons.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants