Skip to content
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

Merged
merged 2 commits into from
Feb 9, 2024
Merged

Conversation

jakelishman
Copy link
Member

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.

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.
@coveralls
Copy link

coveralls commented Feb 8, 2024

Pull Request Test Coverage Report for Build 7844408343

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 96.568%

Totals Coverage Status
Change from base Build 7833941903: 0.0%
Covered Lines: 16740
Relevant Lines: 17335

💛 - Coveralls

Copy link
Collaborator

@IvanIsCoding IvanIsCoding left a 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 = [
Copy link
Collaborator

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

Copy link
Member Author

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?

Copy link
Member Author

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)

Copy link
Collaborator

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

@IvanIsCoding IvanIsCoding added the automerge Queue a approved PR for merging label Feb 9, 2024
@mergify mergify bot merged commit 5a99100 into Qiskit:main Feb 9, 2024
27 checks passed
@jakelishman jakelishman deleted the tox-lint-jobs branch February 9, 2024 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Queue a approved PR for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants