Lints all Python files via flake8 #62
Conversation
conorsch
force-pushed
the
56-linting-flake8
branch
2 times, most recently
from
9495499 to
57f609d
Compare
Runs flake8 to lint all Python files. Deferring to the `flake8` commands on which files are Python, rather than explicitly passing a whitelist of files to lint. Users must install at least v3.3.0 of flake8; older versions, such as 2.5.5 (currently available in the Fedora 25 EOL repos), are not sufficient for the file discovery logic. As a stopgap, we can pass "." for the current working directory to flake8, which will force even older versions to traverse for file discovery. Note that we're *not* setting the `assert-dom0` target as a dependency, since dom0 will not have flake8 installed by default. It's reasonable for the dev env to have it, and we'll make sure the linting checks run in CI.
Mostly whitespace violations, cleaned up with consistent indentation throughout. All files under the tests/ directory now pass flake8 linting. There are a few minor exceptions added, to permit lines longer than 80 characters to aid in readability, e.g. filepaths. Found one major error, in the decryption tests: two tests sharing the same name, so there was clobbering going on. Resolved. Hurray, linting!
The Python scripts that handle state monitoring inside the SD Journalist VM now conform to flake8 linting standards. The changes were minimal. Permitted a longer-80-char line to preserve readability on a URL.
Single commit for resolution of the E741 violation, which states that "l" (i.e. lowercase L) should not be used as a variable name, since it can be mistaken visually for a "1" (i.e. the number one). That's a bit of a stretch in my mind, but hey, let's trust the linter. The first version of flake8 including this check is v3.5.0. Older versions will silently ignore the violation.
There was a bare except in the _reboot function, which would mask errors of any kind. As stipulated by Flake8 E722, "except" blocks should catch explicitly declared exceptions. Since this codeback isn't currently used anywhere—it's commented out in the setUp def—let's remove the noncompliant try/except block and watch for errors down the road. When observed, we'll catch the errors by name and handle appropriately.
Using a clean Debian Stretch container image and customizing it slightly to pull in flake8 via pip. Not currently pinning any pip lib versions, e.g. via requirements.txt, although we'll surely want to in the future. Not using any of the FPF-maintained container images for CI [0], since they all assume Python3, and the Qubes management code is all still Python2 for the time being.
conorsch
force-pushed
the
56-linting-flake8
branch
from
f78e1b9 to
b77b772
Compare
|
Caught two more violations with a more recent version of |
|
Enabled branch protection on |
|
This PR misses all the scripts in the repo, and just modifies the unittest Will circle back on those and submit a separate PR, to aid in review here. Please proceed with review. |
|
Thank you for review, @joshuathayer and @kushaldas! Ready to merge, but looks like the CircleCI logic is a bit wonky:
Hey @msheiny, any bright ideas to resolve? I don't mind temporarily disabling the branch protection in order to merge—the |
|
heyy @conorsch quick check did you rename/rebase this PR a few times during the circle CI shake out? im guessing at one point the job name was updated and in github you have it pegged to the previous name. Solution is easy though!
|
|
You rock, @msheiny, that did it! Merging... |
…lake8 Lints all Python files via flake8
Cosmetic changes throughout, guided by
flake8. The vast majority of changes were whitespace-related—pretty standard for Python projects—although in at least one case, the linter caught a genuine error in code, in which the test nametest_decrypt_sd_submission_desktopwas used twice, causing clobbering. That's resolved now.Note that it's not easy to run
make flake8in the development environment: you'll need to runsudo dnf install python2-flake8in the Work VM in order to use it. The goal is at least to get coverage in CI working, so that we have flake8 validation running prior to every PR merge.Partial completion toward #56.
Marking this PR as "WIP" until I confirm CI is working as expected.Done, CI is passing.