Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

treewide: Reformat all nix blocks in markdown files #299694

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dasJ
Copy link
Member

@dasJ dasJ commented Mar 28, 2024

Description of changes

Follow-up from #299554. Currently a draft since we need to rework CONTRIBUTING.md first (and also add a GH workflow).

cc @drupol @DanielSidhion @fricklerhandwerk since you may be interested in the attached script

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@dasJ
Copy link
Member Author

dasJ commented Mar 28, 2024

Ah also cc @hsjobeki since you said this task would be either hard or impossible (not sure what you said) ;)

@hsjobeki
Copy link
Contributor

hsjobeki commented Mar 28, 2024

Very nice work! If you'd be up for the challenge, we could also check the generated .md files. I thought of those that are generated from nixdoc from doc-comments ( here ) This would ensure doc-comments that declare nix examples would actually evaluate. But i feel like this has some overlap with the doc-tester idea. And didn't look into its details yet.

@dasJ
Copy link
Member Author

dasJ commented Mar 29, 2024

@hsjobeki not sure how well that would work tbh. I checked and I see blocks like:

cli.toGNUCommandLine {} {
  data = builtins.toJSON { id = 0; };
  X = "PUT";
  retry = 3;
  retry-delay = null;
  url = [ "https://example.com/foo" "https://example.com/bar" ];
  silent = false;
  verbose = true;
}
=> [
  "-X" "PUT"
  "--data" "{\"id\":0}"
  "--retry" "3"
  "--url" "https://example.com/foo"
  "--url" "https://example.com/bar"
  "--verbose"
]

cli.toGNUCommandLineShell {} {
  data = builtins.toJSON { id = 0; };
  X = "PUT";
  retry = 3;
  retry-delay = null;
  url = [ "https://example.com/foo" "https://example.com/bar" ];
  silent = false;
  verbose = true;
}
=> "'-X' 'PUT' '--data' '{\"id\":0}' '--retry' '3' '--url' 'https://example.com/foo' '--url' 'https://example.com/bar' '--verbose'";

That's not really valid nix but I see why it is like that, I cannot really change that. Any idea how to improve on that? Maybe split one block into two where the first one is valid nix and the second one a plain one?

@hsjobeki
Copy link
Contributor

how well that would work tbh. I checked and I see bloc

Right seems rather complicated to somehow parse those example since they are all different and cannot be parsed into an AST. Without a proper convention.

We need to introduce a good convention for those examples before we try to parse them.

One idea. Since nix doesn't use the fat arrow => we could split the example there and see if the head is parseable.

Since this => is already kind of established. One difficulty is that we would need some context awareness to make sure => comes after the expression. (Means we have to custom build our own nix example parser 😄 )

@fricklerhandwerk
Copy link
Contributor

fricklerhandwerk commented Mar 30, 2024

@hsjobeki I strongly urge to avoid piling up syntax or mechanisms and just as strongly support @dasJ's suggestion: markdown already parses code blocks, and that can easily be leveraged for making the distinction at the presentation layer with CSS (we do this successfully on nix.dev). It can, with slightly more effort, be used for extracting input-output-pairs for tests.

Example:

```nix
1 + 1
```

```console
2
```

We don't even have to fix that now. Should be enough to agree on a general direction and merge strict improvements on that path independently.

@hsjobeki
Copy link
Contributor

hsjobeki commented Mar 30, 2024

👍 That's what I was implying here. It left much room for interpretations ^^.
With the comment I wanted to express that this problem is way too complex to be solved with just this PR but i am totally agreeing in what direction this goes.

Copy link
Member

Choose a reason for hiding this comment

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

I have comments to make here, but would prefer merging all the reformat work separately so it doesn't get stale. What do you think of moving this script into its own PR so we can iterate on it a little bit?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, if it helps you as a standalone script for iteration :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

5 participants