dev(ruff): added flake8-comprehensions and flake8-annotations to ruff #12399
Conversation
sagar-salvi-apptware
changed the title
github-actions
bot
added
the
ingestion
metadata-ingestion/pyproject.toml
Outdated
| "C401", # Unnecessary generator | ||
| "C403", # Unnecessary list comprehension | ||
| "C408", # Unnecessary `list()` call | ||
| "C416", # Unnecessary Comprehension |
There was a problem hiding this comment.
What's the reasoning behind adding C4 if we're going to ignore a large fraction of the rules?
Also, many of the rules are auto-fixable. If these are genuinely rules that we want, we should just fix all the existing code
There was a problem hiding this comment.
Hi @hsheth2 ,
The main reason for adding this is to improve the readability and quality of list, set, and dict comprehensions.
Out of the 19 C4 rules:
- 10 are supported and have been implemented.
- 4 can be addressed either now or later.
- 5 are not needed at the moment.
There was a problem hiding this comment.
The PR has been updated to address support for C401 and C403.
Changes for C408 and C416 require more extensive modifications and we can handled at a later stage.
Thanks.
sagar-salvi-apptware
force-pushed
the
fix/ING-791-added-flake8-compr-annotations
branch
from
2f4f2cb to
f265f36
Compare
| @@ -38,10 +40,18 @@ extend-ignore = [ | |||
| "E203", # Ignore whitespace before ':' (matches Black) | |||
| "B019", # Allow usages of functools.lru_cache | |||
| "B008", # Allow function call in argument defaults | |||
| "COM812", # Avoid conflicts with formatter | |||
There was a problem hiding this comment.
why did we need to add this here?
| "C410", # Ignore Unnecessary list literal passed to `list()` | ||
| "C414", # Ignore Unnecessary `reversed()` call within `sorted()` | ||
| "C417", # Ignore Unnecessary `map()` usage (rewrite using a list comprehension) | ||
| "C419", # Ignore Unnecessary list comprehension |
There was a problem hiding this comment.
seems like all of these should also be in the "enable later" section
we should also explain why each "enable later" rule was not enabled now
| @@ -113,9 +113,9 @@ def check_path_specs_and_infer_platform( | |||
| raise ValueError("path_specs must not be empty") | |||
|
|
|||
| # Check that all path specs have the same platform. | |||
| guessed_platforms = set( | |||
| guessed_platforms = { | |||
There was a problem hiding this comment.
imo explicitly saying set here is way more clear - I actually dislike this change
in general - I don't like any of the changes that replace set() calls with { x, y } set literal syntax
|
Was thinking about this a bit more - I don't think we want these rule sets yet. |
Checklist