-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Adds a zero_disc_if_single_disc to the zero plugin #6015
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
base: master
Are you sure you want to change the base?
Conversation
Adds a zero_disc_number_if_single_disc boolean to the zero plugin for writing to files. Adds the logic that, if disctotal is set and there is only one disc in disctotal, that the disc is not set. This keeps tags cleaner, only using disc on multi-disc albums. The disctotal is not touched, particularly as this is not usually displayed in most clients. The field is removed only for writing the tags, but the disc number is maintained in the database to avoid breaking anything that may depend on a disc number or avoid possible loops or failed logic.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideIntroduce a zero_disc_if_single_disc option in the zero plugin that omits the disc tag for single-disc albums during tag writing. Sequence diagram for omitting disc tag on single-disc albums during tag writingsequenceDiagram
participant ZeroPlugin
participant Item
participant Tags
ZeroPlugin->>Item: Check item.disctotal
alt zero_disc_if_single_disc enabled and disctotal == 1
ZeroPlugin->>Tags: Set "disc" to None (omit disc tag)
else
ZeroPlugin->>Tags: Leave "disc" tag unchanged
end
Class diagram for updated Zero plugin configuration and logicclassDiagram
class ZeroPlugin {
+fields: list
+keep_fields: list
+update_database: bool
+zero_disc_if_single_disc: bool
+set_fields(item, tags)
}
ZeroPlugin : set_fields(item, tags)
ZeroPlugin : zero_disc_if_single_disc
ZeroPlugin : fields
ZeroPlugin : keep_fields
ZeroPlugin : update_database
class Item {
+disc: int | str
+disctotal: int
}
ZeroPlugin --> Item : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `beetsplug/zero.py:128` </location>
<code_context>
fields_set = False
+ if "disc" in tags and self.config["zero_disc_if_single_disc"].get(bool) and item.disctotal == 1:
+ self._log.debug("disc: {.disc} -> None", item)
+ tags["disc"] = None
+
</code_context>
<issue_to_address>
**suggestion:** The debug log message could be more descriptive.
Include the reason for zeroing the disc field in the log, such as noting it's a single-disc release and the config option is enabled, to improve clarity for future debugging.
```suggestion
self._log.debug(
"Zeroing disc field: single-disc release (disctotal=1) and 'zero_disc_if_single_disc' config enabled. disc: {.disc} -> None",
item
)
```
</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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
Overall this looks good. Im not too sure about the name for the config option tho. It seems a bit wordy to me and I don't think we need the zero in there as this is a plugin specific option.
How about something simpler like omit_single_disc
?
Change the parameter name to omit_single_disc (vs previously zero_disc_if_single_disc) Add return of 'fields_set' so that, if triggered by the command line `beets zero`, it will still effect the item.write. Added tests.
@semohr Thank you for the suggestion. It has been implemented changing the parameter name to omit_single_disc (vs previously zero_disc_if_single_disc). Also, with this update, I have:
|
Remove tests. Update docs. Remove unnecessary return.
Add back tests as they were.
Tried to implement and just got more problems. Left it as is for now (could be cleaned up once the migration of tests gets done for these plugins) unless you'd rather just remove the tests entirely. Ideally just want to merge this important feature and move forward so it doesn't miss out on another release of Beets and aren't chasing other people's merges. |
Adds a
zero_disc_if_single_disc
boolean to the zero plugin for writing to files. Adds the logic that, if disctotal is set and there is only one disc in disctotal, that the disc is not set.This keeps tags cleaner, only using disc on multi-disc albums. The disctotal is not touched, particularly as this is not usually displayed in most clients.
The field is removed only for writing the tags, but the disc number is maintained in the database to avoid breaking anything that may depend on a disc number or avoid possible loops or failed logic.
A column of disc 1 makes me feel there should be a disc 2, when most albums are a single disc only.
NOTE: This was originally at #5909 but I managed to screw up the source/patch repo while trying to bring things up to date with master, so I just recreated the final version of it. I've also changed the parameter to remove the 'number' which I'm told won't always translate well (and technically the disc could be a letter such as 'A').
Description
This addresses concerns such as https://beets-users.narkive.com/fxh0cgmC/beets-remove-disc-field-for-single-disc-albums and accomplishes things like #5670 but in tags instead of just in the path.
To Do
Summary by Sourcery
Introduce a zero_disc_if_single_disc configuration to the zero plugin, skipping the disc tag on single-disc albums when writing tags.
New Features:
Documentation:
Summary by Sourcery
Add an option to the zero plugin that omits the disc tag when only one disc is present, keeping tags lean for single-disc albums.
New Features:
Documentation: