Annoyed with the interference of the python formatting of
generated code (see #72964), I took matters into my own hands
as maintainer of dockerTools.
Afterwards, I've created a PR, hoping to unstuck the discussion.
@aszlig took notice and thanks to his python ecosystem knowledge,
the testing efforts of @blaggacao and @Ma27, and a sense of
shared suffering and comraderie we were able to change the
situation for the better in #122201.
Now, we have a proper linter that actually helps contributors,
so it's time to turn it back on again.
I'm glad we could make it happen this quickly!
Thanks!
This reverts commit 4035049af3.
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]: https://github.com/NixOS/nixpkgs/issues/72964
[2]: https://github.com/NixOS/nixpkgs/pull/122197
[3]: https://github.com/NixOS/nixpkgs/pull/122197#pullrequestreview-654997723
There were a bunch of unnecessary f-strings in there and I also removed
the "# fmt: on/off" comments, because we no longer use Black and thus
won't need those comments anymore.
Signed-off-by: aszlig <aszlig@nix.build>
The new linter basically does
def testScript
# ...
before calling `pyflakes`. As this test-script is empty, it would lead
to a syntax-error unless `pass` is added.
Our test driver exposes a bunch of variables and functions, which
pyflakes doesn't recognise by default because it assumes that the test
script is executed standalone. In reality however the test driver script
is using exec() on the testScript.
Fortunately pyflakes has $PYFLAKES_BUILTINS, which are the attributes
that are globally available on all modules to be checked. Since we only
have one module, using this environment variable is fine as opposed to
my first approach to this, which tried to use the unstable internal API
of pyflakes.
The attributes are gathered by the main derivation of the test driver,
because we don't want to end up defining a new attribute in the test
driver module just to being confused why using it in a test will result
in an error.
Another way we could have gathered these attributes would be in
mkDriver, which is where the linting takes place. However, we do have a
different set of Python dependencies in scope and duplicating these will
again just cause confusion over having it at one location only.
Signed-off-by: aszlig <aszlig@nix.build>
Co-Authored-By: aszlig <aszlig@nix.build>
