-
Notifications
You must be signed in to change notification settings - Fork 145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve ergonomics of tox
lint jobs
#1084
Conversation
This fixes the `lint` job for `tox>=4`, which uses `whitelist_externals` is removed in favour of `allowlist_externals`. This also makes the `lint` and `black` runs skip installation of the package, which makes them much faster to use while making changes, and these are static analysis tools, so don't need to install.
Pull Request Test Coverage Report for Build 7844408343
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, I had the same ideas for #861. Just a minor ruff question
@@ -9,7 +9,7 @@ target-version = ['py38', 'py39', 'py310', 'py311'] | |||
[tool.ruff] | |||
line-length = 105 # more lenient than black due to long function signatures | |||
src = ["rustworkx", "setup.py", "retworkx", "tests"] | |||
select = [ | |||
lint.select = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really necessary? I don't see the advantage of doing this. We only use ruff for linting so it is implicit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ruff issued a warning asking me to do it when I ran the job (same with the other change to a table name) - I think they were planning to deprecate top-level select maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For context, since I'm sat at my computer now, here's what the tox job says without the two extra lint.
bits, from a fresh environment rebuild:
lint: remove tox env folder /Users/jake/code/retworkx/.tox/lint
lint: install_deps> python -I -m pip install -c/Users/jake/code/retworkx/constraints.txt -U 'black~=22.0' ruff
lint: commands[0] /Users/jake/code/retworkx/tests> black --check --diff ../rustworkx ../tests ../retworkx
All done! ✨ 🍰 ✨
160 files would be left unchanged.
lint: commands[1] /Users/jake/code/retworkx/tests> ruff check ../rustworkx ../retworkx . ../setup.py
warning: The top-level linter settings are deprecated in favour of their counterparts in the `lint` section. Please update the following options in `/Users/jake/code/retworkx/pyproject.toml`:
- 'select' -> 'lint.select'
- 'per-file-ignores' -> 'lint.per-file-ignores'
lint: commands[2] /Users/jake/code/retworkx/tests> cargo fmt --all -- --check
lint: commands[3] /Users/jake/code/retworkx/tests> python /Users/jake/code/retworkx/tools/find_stray_release_notes.py
lint: OK (16.18=setup[5.48]+cmd[4.62,0.56,5.17,0.36] seconds)
congratulations :) (16.42 seconds)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess they decided to change the name
This fixes the
lint
job fortox>=4
, which useswhitelist_externals
is removed in favour ofallowlist_externals
. This also makes thelint
andblack
runs skip installation of the package, which makes them much faster to use while making changes, and these are static analysis tools, so don't need to install.