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

Pivot to from flake8 to ruff for Python linting #1006

Merged
merged 3 commits into from
Oct 16, 2023

Conversation

mtreinish
Copy link
Member

Since the start of the 0.3.x release of the retworkx project we have been using flake8 for Python linting and this has worked well for us. However, recently a new project, ruff [1][2], has shown increasing popularity becuase it provides the same linting coverage but is signficantly faster. Also just from a language solidatry PoV ruff is also written in rust. This commit migrates our linting jobs to use ruff instead of flake8 and sets up a reasonable set of rules as a starting point, which includes the rules equivalent to what we were using before with flake8, pyupgrade, and flake8-pyi, and flake8-quotes.

[1] https://docs.astral.sh/ruff/
[2] https://github.com/astral-sh/ruff

Since the start of the 0.3.x release of the retworkx project we have
been using flake8 for Python linting and this has worked well for us.
However, recently a new project, ruff [1][2], has shown increasing
popularity becuase it provides the same linting coverage but is
signficantly faster. Also just from a language solidatry PoV ruff is
also written in rust. This commit migrates our linting jobs to use ruff
instead of flake8 and sets up a reasonable set of rules as a starting
point, which includes the rules equivalent to what we were using before
with flake8, pyupgrade, and flake8-pyi, and flake8-quotes.

[1] https://docs.astral.sh/ruff/
[2] https://github.com/astral-sh/ruff
@coveralls
Copy link

coveralls commented Oct 15, 2023

Pull Request Test Coverage Report for Build 6539876916

  • 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.523%

Totals Coverage Status
Change from base Build 6537336187: 0.0%
Covered Lines: 15544
Relevant Lines: 16104

💛 - 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.

This is great and it is a welcome addition in my opinion. ruff seems to work out-of-the-box with black and the fact that it integrates with pyproject.toml by default is nice.

Just three opinions:

  • For the changes in the .pyi to use the non-deprecated aliases, it's worth changing BUT Python 3.8 holds us back
  • I agree with PYI001 warnings and I think we should fix them in a separate PR
  • We should put this on hold until Ramp up deprecation of retworkx package and remove tests #1004 is merged to avoid merge conflict headaches

pyproject.toml Show resolved Hide resolved
rustworkx/digraph.pyi Show resolved Hide resolved
"rustworkx/__init__.pyi" = ["F403", "F405", "PYI001"]
"rustworkx/digraph.pyi" = ["F403", "F405", "PYI001"]
"rustworkx/graph.pyi" = ["F403", "F405", "PYI001"]
"rustworkx/iterators.pyi" = ["F403", "F405", "PYI001"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should fix PYI001 in those files, it would avoid us having to define __all__ in them

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.

LGTM, we have the from __future__ import annotations so the change to typing stubs are fine

@IvanIsCoding IvanIsCoding added the automerge Queue a approved PR for merging label Oct 16, 2023
@mergify mergify bot merged commit b1537d9 into Qiskit:main Oct 16, 2023
27 checks passed
@mtreinish mtreinish deleted the use-ruff branch October 16, 2023 22:41
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.

None yet

3 participants