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

nixos/manual/writing-nixos-tests: document how to disable Black silently #115879

Merged
merged 1 commit into from
Mar 11, 2021

Conversation

khumba
Copy link
Contributor

@khumba khumba commented Mar 11, 2021

Motivation for this change

This is following up on a request from @flokli in #96515 to add documentation for disabling the Black linter for NixOS tests without generating a warning (re. #72964).

I've built the manual with the two commands from the manual to verify the changes:

make -C nixos/doc/manual
nix-build nixos/release.nix -A manual.x86_64-linux
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
    • I don't see any, but I've run the manual's commands for itself, as above.
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
    • Nope, Git has problems with this patch apparently and nixpkgs-review's call to it chokes. Magit was having issues too, kinda weird.
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
    • That's all this PR does :).
  • Fits CONTRIBUTING.md.

@khumba khumba changed the title nixos/manual/writing-nixos-tests: document how to Black silently nixos/manual/writing-nixos-tests: document how to disable Black silently Mar 11, 2021
@Ma27
Copy link
Member

Ma27 commented Mar 11, 2021

I actually think this is fine to use in nixpkgs if you have e.g. foo.succeed("${something-that-evaluates-to-a-store-path}") that would be short enough for one line or did I miss some recent discussion?

@flokli
Copy link
Contributor

flokli commented Mar 11, 2021

No, this is not okay, as it disables all linting for the whole file.

@flokli
Copy link
Contributor

flokli commented Mar 11, 2021

There's #72964, which proposes switching to less stricter linting, so these kind of interpolations are not too much of a problem, but we shouldn't encourage disabling linting alltogether.

@flokli flokli merged commit 540af5f into NixOS:master Mar 11, 2021
@flokli
Copy link
Contributor

flokli commented Mar 11, 2021

Thanks for the PR!

@Ma27
Copy link
Member

Ma27 commented Mar 11, 2021

@flokli I thought #fmt: on re-activates it oO

@khumba
Copy link
Contributor Author

khumba commented Mar 11, 2021

@Ma27 # fmt: on does reactivate it past that point. Although it's the end of the file and it's not strictly needed for Black to be satisfied, I included it to work around an issue I mentioned here where if you leave it off, Black will still complain about extra newlines at the end of the file. You get those if you interpolate a '' string at the end of testScript. For the tests I've written (not in Nixpkgs), I hit that 100% of the time because I have a used a wrapper function to do that. "# fmt: off\n${testScript}" would work instead but it's not as ergonomic.

(edited to add missing quote)

@khumba
Copy link
Contributor Author

khumba commented Mar 11, 2021

And thanks for the suggestion @flokli!

@Ma27
Copy link
Member

Ma27 commented Mar 11, 2021

Ahh, I see. In that case it should indeed be avoided in nixpkgs, thanks for the clarification.

@khumba khumba deleted the doc-black-disable branch March 11, 2021 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants