search

pop-os / system76-driver

System76 Driver for Pop!_OS
Other
112 stars 29 forks source link

lint using flake8 #223

Closed danieleades closed 2 years ago

danieleades commented 2 years ago

adds a pre-commit hook and corresponding github workflow to lint using flake8

i've addressed a couple of lints, but for the most part i've suppressed the errors, so that they can be adopted incrementally. Most of them are related to formatting, and could be resolved in one fell swoop by using a pep8-compliant formatter.

danieleades commented 2 years ago

you can see the job status here - https://github.com/danieleades/system76-driver/runs/4341539076?check_suite_focus=true

danieleades commented 2 years ago

On projects with a long history supporting over a hundred systems like this, formatting can be devastating. It can introduce random regressions and clobbers tools like git blame. I cannot accept this.

that seems an oddly restrictive approach. The changes in this PR are very conservative.

It can introduce random regressions

The usual approach to mitigate the risk of regression is to introduce more continuous integration quality gates, not less. Rejecting refactoring exercises, and sticking to manual testing clearly isn't scalable, at least in the general case. Maybe it's acceptable here if you expect the codebase to be basically stable from now to eternity.

clobbers tools like git blame

you can restrict the lints to only check for code correctness and not code formatting. In fact, I've suppressed the majority of formatting lints in this PR already. Regardless, I certainly don't get any strong impression that this PR or others like it would receive a particularly warm welcome had those formatting lints been fully suppressed. possibly that's unfair.

jackpot51 commented 2 years ago

We already have automated tests in the system76driver/tests folder and linting using pyflakes3. Please don't take this personally, we just don't need the changes you have made in this PR.