nixos/testing-python.nix: Add skipFormatter #122197
Conversation
github-actions
bot
added
the
6.topic: nixos
| , skipLint ? false | ||
| # Skip formatting check | ||
| , skipFormatter ? false |
There was a problem hiding this comment.
Having this false by default wouldn't prevent those annoyances (you get annoyed, then enable skipFormatter until all tests have it enabled?) unless we use skipFormatter = true; for every test, which then in turn poses the question why we need black at all.
There was a problem hiding this comment.
You're 100% right about this, but the apparent lack of agreement on this topic still makes me hesitant to decide for everyone. I'd be happy to change it to true though.
ofborg
bot
added
10.rebuild-darwin: 0
| @@ -158,7 +160,7 @@ rec { | |||
| mkdir -p $out/bin | |||
|
|
|||
| echo -n "$testScript" > $out/test-script | |||
There was a problem hiding this comment.
Hm, what about doing something like this instead:
| echo -n "$testScript" > $out/test-script | |
| echo "# fmt: off" > $out/test-script | |
| echo -n "$testScript" >> $out/test-script |
I do this by default on all my tests and if I'm not missing something, this causes black to not apply formatting but still does things such as syntax checks and other rather useful things.
There was a problem hiding this comment.
What else does it do? I've re-read the README, but it only seems to talk about formatting.
There was a problem hiding this comment.
Giving this a shot here #122199
There was a problem hiding this comment.
Okay, just checked: It basically just does a syntax check. Maybe using something like pyflakes is a better idea because it does have useful checks, eg. usage of undefined variables, invalid format arguments, duplicate arguments, shadowed loop vars and more.
There was a problem hiding this comment.
As of now, this only adds an option that does exactly the same as the already existing one.
black does more than linting, yes. Last September it was proposed to switch from black to to a more permissive (only-)linter.
I don't think adding another option (skipFormatter) that currently does exactly the same as skipLint will help out of this confusion.
IMHO, we should keep skipLint, but use a linter that doesn't format, at least not enforce the line length (due to the nix interpolation we do).
|
Closing in favor of #122201 |
This switches the linting of the NixOS test driver script from Black (which is a code *formatter*) to PyFlakes. Contrary to Black, which only does formatting and a basic syntax check, PyFlakes actually performs useful checks early on before spinning up VMs and evaluating the actual test script and thus becomes actually useful in development rather than an annoyance. One of the reasons why Black has been an annoyance[1] was because it assumed that the files that it's formatting aren't inlined inside another programming language. With NixOS VM tests however, we inline these Python scripts in the testScript attribute. With some of them using string antiquotations, things are getting rather ugly because Black now no longer formats static code but generated code from Nix being used as a macro language. This becomes especially annoying when an antiquotation contains an option definition from the NixOS module system, since an unrelated change might change its length or contents (eg. suddenly containing a double quote) for which Black will report an error. While issue #72964 has been sitting around for a while (and probably annoyed everyone involved), nobody has actually proposed an implementation until @roberth did a first pull request[2] yesterday which added a "skipFormatter" attribute, which contrary to skipLint silently disabled Black. This has led to very legitimate opposition[3] from @flokli: > As of now, this only adds an option that does exactly the same as the > already existing one. > > black does more than linting, yes. Last September it was proposed to > switch from black to to a more permissive (only-)linter. > > I don't think adding another option (skipFormatter) that currently > does exactly the same as skipLint will help out of this confusion. > > IMHO, we should keep skipLint, but use a linter that doesn't format, > at least not enforce the line length (due to the nix interpolation we > do). This was written while I was doing an alternative implementation and pretty much sums up the work I'm merging here, which switches to PyFlakes, which only checks for various errors in the code (eg. undefined variables, shadowing, wrong comparisons and more) but does not do *any* formatting. Since Black didn't do any of the checks performed by PyFlakes (except a basic syntax check), the existing test scripts needed to be fixed. Thanks to @blaggacao, @Ma27 and @roberth for helping with testing and fixing those scripts. [1]: #72964 [2]: #122197 [3]: #122197 (review)
Motivation for this change
Allow skipping the formatter without skipping the linters.
Python's
blackis not a linter, but a formatter.-- the black readme
Linting is a broader and more useful concept, with the goal of improving code rather than enforcing syntactic preferences.
-- wikipedia
Unlike formatters, linters convey useful knowledge about potential problems, so those are actually useful.
That's not to say that enforcing a style isn't useful in general; it's just that it doesn't work well for our generated code. The actual process of formatting can not currently be automated, so the annoyance of matching the expected layout by hand outweighs the annoyance of reading code in an inconsistent style sometimes.
Contributing to Nixpkgs shouldn't be needlessly unpleasant, especially if we want to be serious about testing. (Which we do)
Many developers don't enjoy writing tests in their free time, yet tests are crucial for the ability to improve existing code.
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)