-
Notifications
You must be signed in to change notification settings - Fork 2k
Enable ruff's future-annotations and RUF* rules #6245
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
Conversation
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
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.
Hey - I've found 1 security issue, 2 other issues, and left some high level feedback:
Security issues:
- Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- The change to
BeetsPlugin/BeetsPluginMetaswitchestemplate_funcs/template_fields/album_template_fieldsfrom shared class-level dicts to per-instance dicts; double-check whether any existing plugins rely on cross-instance sharing of these registries and, if so, consider keeping a central class-level store in addition to per-instance overrides. - In several places the new
[*seq, elem]pattern is used where the original code relied on list concatenation semantics (seq + [elem]); it may be worth standardizing on a single style (e.g.,[*seq, elem]everywhere) for consistency and to make the refactor’s intent clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The change to `BeetsPlugin`/`BeetsPluginMeta` switches `template_funcs`/`template_fields`/`album_template_fields` from shared class-level dicts to per-instance dicts; double-check whether any existing plugins rely on cross-instance sharing of these registries and, if so, consider keeping a central class-level store in addition to per-instance overrides.
- In several places the new `[*seq, elem]` pattern is used where the original code relied on list concatenation semantics (`seq + [elem]`); it may be worth standardizing on a single style (e.g., `[*seq, elem]` everywhere) for consistency and to make the refactor’s intent clearer.
## Individual Comments
### Comment 1
<location> `pyproject.toml:314-316` </location>
<code_context>
"N", # pep8-naming
"PT", # flake8-pytest-style
- # "RUF", # ruff
+ "RUF", # ruff
"UP", # pyupgrade
- "TCH", # flake8-type-checking
+ "TC", # flake8-type-checking
"W", # pycodestyle
]
</code_context>
<issue_to_address>
**issue (bug_risk):** The `TC` code in Ruff `select` likely doesn’t correspond to flake8-type-checking
Ruff uses the `TCH` prefix for flake8-type-checking rules (e.g., `TCH001`), not `TC`, so this entry won’t enable the plugin as intended. Update the code to use `"TCH"` instead of `"TC"` here.
</issue_to_address>
### Comment 2
<location> `beets/util/pipeline.py:195` </location>
<code_context>
task: R | T | None = None
while True:
task = yield task
- task = func(*(args + (task,)))
+ task = func(*((*args, task)))
return coro
</code_context>
<issue_to_address>
**suggestion:** The new argument expansion in `coro` is more complex than necessary and slightly obscures intent
`func(*((*args, task)))` is equivalent to `func(*args, task)` but less readable and creates an unnecessary tuple. Please simplify to `func(*args, task)` here and in the similar `coro` call below.
</issue_to_address>
### Comment 3
<location> `beetsplug/mbsubmit.py:72` </location>
<code_context>
subprocess.Popen([picard_path, *paths])
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
40adecc to
97fe1f1
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6245 +/- ##
==========================================
+ Coverage 68.68% 68.91% +0.23%
==========================================
Files 138 138
Lines 18540 18553 +13
Branches 3064 3061 -3
==========================================
+ Hits 12734 12786 +52
+ Misses 5152 5116 -36
+ Partials 654 651 -3
🚀 New features to boost your workflow:
|
97fe1f1 to
2d0b8f8
Compare
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.
Pull request overview
This PR updates typing and linting across the codebase to enable stricter ruff checks for Python 3.10+. It automatically adds from __future__ import annotations where needed, enforces PEP 604 union syntax, and addresses various linting issues including proper use of ClassVar for mutable class attributes.
Key changes:
- Enabled
ruff'sfuture-annotationsfeature (auto-addsfrom __future__ import annotationsand moves imports underTYPE_CHECKING) - Set
target-version = "py310"to enforce PEP 604 union syntax (str | Noneinstead ofUnion[str, None]) - Enabled
RUF*rules and addressed all identified issues (unused# noqa, unused unpacked variables, list materialization,ClassVarannotations)
Reviewed changes
Copilot reviewed 79 out of 80 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Updated ruff version requirement and configuration to enable future-annotations, set py310 target, and enable RUF rules |
| .git-blame-ignore-revs | Added commit hashes for the linting/typing changes to git blame ignore list |
| Multiple test files | Replaced unused variables with _ placeholder and removed unnecessary # noqa comments |
| Multiple plugin files | Added ClassVar type hints to mutable class attributes and moved imports under TYPE_CHECKING |
| beets/plugins.py | Introduced BeetsPluginMeta metaclass to properly distinguish class vs instance attributes for template fields |
| beets/dbcore/*.py | Added ClassVar annotations to class-level field/sort dictionaries and moved type imports |
| beets/library/models.py | Added ClassVar annotations to _fields, _sorts, and item_keys dictionaries |
| Multiple source files | Replaced list concatenation with unpacking syntax ([*list, item]) and converted to PEP 604 union types |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
JOJ0
left a comment
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.
|
damnit I used the wrong github account for the review. It's me as you've probably guessed ;-) |
henry-oberholtzer
left a comment
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.
Looks good to me - doesn't look like it'll conflict terribly with any existing PR's either, if at all.
2d0b8f8 to
78d8bc8
Compare
78d8bc8 to
177cde2
Compare
177cde2 to
c9625f8
Compare


Summary
This PR updates typing and linting across the codebase and enables stricter
ruffchecks for Python 3.10:Enable
tool.ruff.lint.future-annotationsVery handy feature released in
0.13.0: if required, it automatically addsfrom __future__ import annotationsand moves relevant imports underif TYPE_CHECKING:Set
tool.ruff.target-version = "py310"This enforced PEP 604 unions in the codebase:
Enable
RUF*family of checksRemove unused
# noqasIgnore unused unpacked variables
Avoid list materialization
And, most importantly, RUF012: use
ClassVarfor mutable class attributesBeetsPlugin.template_*attributes design, where I have now definedBeetsPluginMetato make a clear distinction between class and instance attributes. @semohr and @asardaes I saw you had a discussion regarding these earlier - we will need to revisit this at some point to sort it out for good.metasync.MetaSourcewhereitem_typeswere initialised as an instance attribute (but luckily never used).