Add linting support #147
Add linting support #147
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #147 +/- ##
==========================================
+ Coverage 66.13% 66.34% +0.21%
==========================================
Files 24 24
Lines 1680 1682 +2
==========================================
+ Hits 1111 1116 +5
+ Misses 569 566 -3
☔ View full report in Codecov by Sentry. |
Include flake8 and pylint as dev dependency. Add configuration file for pylint and flake8, for the moment all warnings currently present in the HEAD are disabled. Add lint targets to the make project. Add lint stage in github pipeline.
Enable most of the flake8 tests. Fix simple problems from flake8 report.
Remove logging lint warning disabling Add lazy formatting where needed
Fix warning about unused code Fix warning about imports Fix warning about too complex if/else
Yes they are from pylint, in |
.github/workflows/ci.yml
Outdated
| - name: Linting | ||
| run: make lint | ||
| - name: Run tests + cov report | ||
| run: make test-with-coverage | ||
| - name: Check style | ||
| run: make checkstyle | ||
| run: make black |
There was a problem hiding this comment.
I would combine those style checks so
- name: Run tests + cov report
run: make test-with-coverage
- name: Check style
run: make checkstyle
so effectively just leave it as is as you added lint+flake8 to checkstyle
There was a problem hiding this comment.
I think it make sense for the user to have output from black separated from the one from (pylint and flake8)
pylint and flake8 also have some warning about formatting and indentation that black will detect as error too.
But the part I like the user perceive as lint is all the warning not about indentation like: unused variables, duplicated code, too complex code, ...
In order of importance the code should pass:
- static analysis (pylind flake8)
- runtime analysis (pytest, coverage)
- code formatting (black)
@okurz what do you think about this?
There was a problem hiding this comment.
Ah, I guess the both of us had a similar idea. In #148 I actually used flake8-black which avoids having to call multiple tools and instead integrates them
There was a problem hiding this comment.
pylint and flake8 also have some warning about formatting and indentation that black will detect as error too.
You mean part of the checks are redundant? That's certainly true. But I don't understand why then you would need to separate.
In order of importance the code should pass …
hm, I consider black always part of "static analysis". Another reason why I suggest to just call the meta-target checkstyle in CI as well is to make sure the target actually works to prevent errors like in #147 (comment) :)
There was a problem hiding this comment.
ok good for me to so :-)
mpagot
force-pushed
the
linter
branch
2 times, most recently
from
9eb1c68
to
3e03e61
Compare
Rename target name for black execution from checkstyle to black Add aggregate lint and black under checkstyle target
Include flake8 and pylint as dev dependency.
Add configuration file for pylint and flake8, for the moment all
warnings currently present in the HEAD are disabled.
Add lint targets to the make project.
This commit also result in the github action to run the linter.
Fix simplest warnings.
Verification: https://github.com/openSUSE/qem-bot/actions/runs/6553300955/job/17798390468?pr=147#step:5:1