Skip to content

nixos/python-test-driver: add an option to disable python linter#76171

Merged
worldofpeace merged 1 commit intoNixOS:masterfrom
Ma27:optionally-disable-python-lint
Dec 23, 2019
Merged

nixos/python-test-driver: add an option to disable python linter#76171
worldofpeace merged 1 commit intoNixOS:masterfrom
Ma27:optionally-disable-python-lint

Conversation

@Ma27
Copy link
Copy Markdown
Member

@Ma27 Ma27 commented Dec 22, 2019

Motivation for this change

While it's a good idea to automate the linting of the python code used
for our tests, I think that it can be quite distracting when hacking on
a NixOS test.

I figured that it might be more convenient to add an option as a
shortcut for this to avoid that everyone needs to dig into the test
driver again.

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)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • 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
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Dec 22, 2019
@Ma27 Ma27 requested a review from flokli December 22, 2019 17:49
@flokli
Copy link
Copy Markdown
Member

flokli commented Dec 22, 2019 via email

While it's a good idea to automate the linting of the python code used
for our tests, I think that it can be quite distracting when hacking on
a NixOS test.

I figured that it might be more convenient to add an option as a
shortcut for this to avoid that everyone needs to dig into the test
driver again.
@Ma27 Ma27 force-pushed the optionally-disable-python-lint branch from f75a7ee to b726617 Compare December 22, 2019 18:33
@worldofpeace
Copy link
Copy Markdown
Contributor

This would make me a bit happier ref #72964. I still think linting when the values are in nix strings not as great.

@Ma27
Copy link
Copy Markdown
Member Author

Ma27 commented Dec 22, 2019

I still think linting when the values are in nix strings not as great.

Tbh I fully agree! This is more or less the middle-ground-solution. I thought that there are probably good reasons to use some linting, but I think that there's no point to use this when actively working on a test.

As this is IMHO an improvement over the current situation - is there anything that keeps this from merging? :)

@worldofpeace
Copy link
Copy Markdown
Contributor

I still think linting when the values are in nix strings not as great.

Tbh I fully agree! This is more or less the middle-ground-solution. I thought that there are probably good reasons to use some linting, but I think that there's no point to use this when actively working on a test.

As this is IMHO an improvement over the current situation - is there anything that keeps this from merging? :)

It's a nice middle-ground improvement. Though, it's a bit funny. Our testing function has an argument to disable a linter, it's unexpected. That being said, I'm going to merge it 😄

Copy link
Copy Markdown
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

Things tested:

Built and read NixOS manual ✔️
Ran a test with skipLint and saw a warning ✔️

@worldofpeace worldofpeace merged commit 9dacdec into NixOS:master Dec 23, 2019
@Ma27 Ma27 deleted the optionally-disable-python-lint branch December 23, 2019 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants