remove black as dev dependency and upgrade ruff#56
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #56 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 8 8
Branches 1 1
=========================================
Hits 8 8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rgaudin
left a comment
There was a problem hiding this comment.
❯ hatch run inv lintall
ruff 0.15.12
warning: The top-level linter settings are deprecated in favour of their counterparts in the `lint` section. Please update the following options in `toto/pyproject.toml`:
- 'ignore' -> 'lint.ignore'
- 'select' -> 'lint.select'
- 'unfixable' -> 'lint.unfixable'
- 'flake8-bugbear' -> 'lint.flake8-bugbear'
- 'flake8-tidy-imports' -> 'lint.flake8-tidy-imports'
- 'isort' -> 'lint.isort'
- 'per-file-ignores' -> 'lint.per-file-ignores'
All checks passed!
would be good to handle this now
On it. Also noticed that Ruff should be invoked to fail CI if any formatting happens. |
|
Indeed that should be fixed as well. |
benoit74
left a comment
There was a problem hiding this comment.
Sorry for realizing this a bit "late", but reading https://docs.astral.sh/ruff/formatter, ruff is split between a formatter and a linter. Calling "ruff check" only calls the linter.
I feel like having a formatter is still something mandatory, for instance I've just replaced "space-based" indentation with tabs, and ruff linter complains the file uses tabs but does not fix this. Ruff formatter fix the file automatically.
This means we probably needs to significantly alter the actions in tasks.py to make the distinction between format, lint and typing. Both with the option to only check or automatically fix what can be (not possible with type checker ATM).
We also need to have a quick look at https://docs.astral.sh/ruff/formatter/#conflicting-lint-rules (I think we are ok with this).
yup. i've taken this into account. changing |
benoit74
left a comment
There was a problem hiding this comment.
This is not exactly what I've understood we should do.
To me we should have calls to both ruff check (with --fix when necessary) and then ruff format (see https://docs.astral.sh/ruff/formatter/#sorting-imports regarding why order matters).
I propose we should have these tasks:
- test, test_cov, report_cov, coverage (unchanged)
format=> callsruff formatlint=> callsruff checktyping=> callpyrightanalyse_all=> callslintthenformat --checkthentypingfix_all=> callslint --fixthenformat
I'm not exactly convinced about naming, but I recommend to drop lintall and checkall actions which are just too confusing.
|
Applied changes for #56 (review) with the only modification being that I changed the name from |
rgaudin
left a comment
There was a problem hiding this comment.
Still getting the warning…
❯ hatch run inv check-format
ruff 0.15.12
warning: The top-level linter settings are deprecated in favour of their counterparts in the `lint` section. Please update the following options in `toto/pyproject.toml`:
- 'ignore' -> 'lint.ignore'
- 'select' -> 'lint.select'
- 'unfixable' -> 'lint.unfixable'
- 'flake8-bugbear' -> 'lint.flake8-bugbear'
- 'flake8-tidy-imports' -> 'lint.flake8-tidy-imports'
- 'isort' -> 'lint.isort'
- 'per-file-ignores' -> 'lint.per-file-ignores'
4 files already formatted
That is indeed weird. Dont' get them locally and I don't see it on CI either. Looking at |
|
Ok, it seems this might be due to your environment. From your output, it seems it's using a different
|
|
my bad |
benoit74
left a comment
There was a problem hiding this comment.
We also need to fix docs/Usage.md to move to analyze.
The split between lint and check hatch environment also doesn't work anymore since analyze-all now needs ruff to be install. The distinctions between these two environments was anyway not that valuable so I would propose to merge them into a single qa environment (which will then mandate more changes in docs/Usage.md)
benoit74
left a comment
There was a problem hiding this comment.
Sorry, but reading this again I've found yet another way to simplify / streamline the tasks names.
- prefix all checks with same
checkprefix:check-format,check-lint,check-type,check-all - prefix all fixes with same
fixprefix:fix-format,fix-lint,fix-all
I also think all tasks should accept additional args (e.g. check-format should, it will "force" --check arg but should accept more args to be passed, typically to run on a single file).
This looks like the most understandable and stable "interface" we should provide.
@rgaudin are you ok with this change?
As a consequence, the QA CI should be split into 3 steps: check-format, check-lint, check-type
|
Absolutely ; a lot better |
Given the |
I would still allow to pass args "as-is". It is true that most flags won't work in one tool or another. But all tools accept a path/filename for instance. We won't indeed split args across tools, this is too ambiguous. |
benoit74
left a comment
There was a problem hiding this comment.
I like it, thank you! Few last small changes and we are probably good. Sorry for the small details but this is a repo which is going to be used "everywhere-Kiwix" so it is better to be in good shape.
| @@ -39,11 +39,11 @@ If you've not already read it, please check our [Policy](./Policy.md) first. | |||
| ❯ inv -l | |||
|
|
|||
| # linting, testing, coverage, static type checks | |||
There was a problem hiding this comment.
Comment could better reflect the list of commands below.
|
|
||
| - Remove `black` from `pyproject.toml` and `tasks.py` | ||
| - Upgrade `ruff` to `0.15.12` | ||
| - Merge `check` and `lint` features into `qa` |
There was a problem hiding this comment.
Mention we've renamed all QA-related invoke tasks
Rationale
This PR removes black as a dev dependency due to conflicts between it and ruff. Accordingly, ruff is upgraded to the latest version (as of today).
Changes
blackfrompyproject.toml,.pre-commit-config.yamlandtasks.pyand CI workflowsruffto0.15.12This fixes #55