importsource: Only register listener if configured#6297
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. |
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
98c3efe to
6f75310
Compare
|
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.
Hey - I've found 1 issue, and left some high level feedback:
- When checking
self.config["suggest_removal"]in__init__, consider using the appropriate config API (e.g..get(bool)/.as_bool()) instead of relying on the truthiness of the ConfigView itself to make the intent explicit and avoid subtle behavior changes if the config layer changes. - The tests access
plugins._instances[0]which assumes a specific plugin ordering and a single instance; if possible, consider narrowing this to the concrete plugin instance more directly (e.g. by type or name) to make the tests more robust to future changes in plugin loading.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When checking `self.config["suggest_removal"]` in `__init__`, consider using the appropriate config API (e.g. `.get(bool)` / `.as_bool()`) instead of relying on the truthiness of the ConfigView itself to make the intent explicit and avoid subtle behavior changes if the config layer changes.
- The tests access `plugins._instances[0]` which assumes a specific plugin ordering and a single instance; if possible, consider narrowing this to the concrete plugin instance more directly (e.g. by type or name) to make the tests more robust to future changes in plugin loading.
## Individual Comments
### Comment 1
<location> `beetsplug/importsource.py:29-30` </location>
<code_context>
- self.register_listener(
- "import_task_choice", self.prevent_suggest_removal
- )
+ # Only register removal suggestion listeners if the feature is enabled
+ if self.config["suggest_removal"]:
+ # In order to stop future removal suggestions for an album we keep
+ # track of `mb_albumid`s in this set.
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard should likely use a boolean value from config instead of relying on truthiness of the config view.
In beets, `self.config["suggest_removal"]` is a `ConfigView`, which is always truthy regardless of the configured boolean. As a result, this condition will always pass and the listeners will be registered even when the feature is disabled. Use the underlying boolean value instead (e.g. `self.config["suggest_removal"].get(bool)` or `.as_bool()`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR optimizes the importsource plugin by conditionally registering event listeners only when the suggest_removal feature is enabled. This follows the principle of not registering unnecessary event handlers when functionality is disabled.
Changes:
- Modified plugin initialization to conditionally register
item_removedandimport_task_choicelisteners based on configuration - Added comprehensive tests to verify listener registration behavior in both enabled and disabled states
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| beetsplug/importsource.py | Wrapped listener registration and attribute initialization in a conditional block that checks the suggest_removal config value |
| test/plugins/test_importsource.py | Added new test class with two tests verifying that listeners and attributes are only created when the feature is enabled |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6f75310 to
82bd488
Compare
|
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. |
and remove the now redundant config sanity check in suggest_removal().
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
6276eba to
690511d
Compare
Description
Does not fix anything, just corrects what the plugin is supposed to do. If the feature is not enabled, registering a listener is not required.
Added 2 new tests that prove this is the case.
Documentation.Changelog.Not required since it is a new and unreleased plugin that has a changelog entry already.