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

Apply more ruff rules #12980

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open

Conversation

DimitriPapadopoulos
Copy link
Contributor

@DimitriPapadopoulos DimitriPapadopoulos commented Sep 28, 2024

Enforce these rules:

Also apply assorted rules from these rulesets:

I find these rules improve consistency and readability. I decided to enforce the rulesets I am familiar with from other projects, and merely apply isolated rules when I fear a ruleset might generate too many false positives.

@DimitriPapadopoulos DimitriPapadopoulos marked this pull request as draft September 28, 2024 09:20
@DimitriPapadopoulos DimitriPapadopoulos force-pushed the ruff branch 8 times, most recently from ab372b9 to 8992b30 Compare September 28, 2024 10:42
@DimitriPapadopoulos DimitriPapadopoulos marked this pull request as ready for review September 28, 2024 10:44
@DimitriPapadopoulos DimitriPapadopoulos force-pushed the ruff branch 6 times, most recently from 2da7cf4 to f242120 Compare September 28, 2024 12:09
@notatallshaw
Copy link
Member

Can you add some details to your PR description which groups you're adding and why.

@DimitriPapadopoulos DimitriPapadopoulos force-pushed the ruff branch 2 times, most recently from 835ee02 to fdd6c53 Compare September 28, 2024 17:10
@pfmoore
Copy link
Member

pfmoore commented Sep 28, 2024

Can you add some details to your PR description which groups you're adding and why.

Agreed. This is turning into a pretty huge PR, all of which is essentially just cosmetic changes. I don't want to get into a big debate over "what rules should we enable" but equally I don't think we should simply enable rules because they are there. Apart from anything else, this will likely cause a lot of merge conflicts with the various PRs we currently have outstanding - and it's difficult enough to find time to progress those without dealing with the admin of what is essentially changes in project policy.

To be frank, I'm currently -1 on this change, unless the rules being enforced have some visible benefit to this project.

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Sep 28, 2024

Most linter rules don't dramatically improve individual pieces of code as such. Yet, you have already enabled some rules. Why?

Like the rules you have already enabled, these new rules marginally improve some pieces of code. More importantly, they globally improve consistency and readability.

Note that recent commits such as 26c6a45 would have been caught by rules such as SIM118.

How about splitting this request into multiple requests?

  1. Apply the rules, but do not enforce them.
  2. Enforce the rules later on, gradually.

Alternatively, I can split into a PR for each ruleset.

@DimitriPapadopoulos
Copy link
Contributor Author

The PR is made of small commits, each one applying a rule. The explanation for each rule is available in the Ruff Rules documentation. Copying the rationale for each rule here doesn't seem useful.

@pypa-bot
Copy link

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

@pypa-bot pypa-bot added the needs rebase or merge PR has conflicts with current master label Sep 28, 2024
There's no evidence that generators are meaningfully faster
than list comprehensions when combined with unpacking.
UP031 Use format specifiers instead of percent format
Using `X | Y` in `isinstance` is slower and more verbose.
RUF010 Use explicit conversion flag
RUF012 Mutable class attributes should be annotated with `typing.ClassVar`
RUF019 Unnecessary key check before dictionary access
EXE002 The file is executable but no shebang is present
PIE810 Call `startswith` or `endswith` once with a `tuple`
PYI032 Prefer `object` to `Any` for the second parameter to `__eq__`
PYI036 The first argument in `__exit__` should be annotated with `object` or `type[BaseException] | None`
PYI036 The second argument in `__exit__` should be annotated with `object` or `BaseException | None`
PYI036 The third argument in `__exit__` should be annotated with `object` or `types.TracebackType | None`
RSE102 Unnecessary parentheses on raised exception
RET501 Do not explicitly `return None` in function if it is the only possible return value
RET503 Missing explicit `return` at the end of function able to return non-`None` value
SLOT000 Subclasses of `str` should define `__slots__`
SIM103 Return the (negated) condition directly
SIM110 Use `return any(getattr(req, option, None) for req in reqs)` instead of `for` loop
SIM118 Use `key in dict` instead of `key in dict.keys()`
TCH001 Move application import `.base.Requirement` into a type-checking block
TCH002 Move third-party import `pip._vendor.resolvelib.structs.DirectedGraph` into a type-checking block
TCH003 Move standard library import `distutils.cmd.Command` into a type-checking block
PGH003 Use specific rule codes when ignoring type issues
PGH004 Use specific rule codes when using `noqa`
C419 Unnecessary list comprehension
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants