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

Add advanced rewrite plugin #4845

Merged
merged 4 commits into from Oct 15, 2023
Merged

Conversation

Maxr1998
Copy link
Contributor

@Maxr1998 Maxr1998 commented Jul 12, 2023

Description

This plugin allows rewriting fields based on a given library query. This can be helpful, for example, when an artist was renamed but you'd like to keep their older releases under their old name, or if you have a single track from a Various Artists release and want to have it included with the original artist.

Example configuration (restores the old name of a recently renamed artist for releases before 2023):

advancedrewrite:
  - match: "mb_artistid:dec0f331-cb08-4c8e-9c9f-aeb1f0f6d88c year:..2022"
    field: artist
    replacement: "이달의 소녀 오드아이써클"

To Do

  • Possibly increasing the plugin's robustness and error handling
  • Documentation.
  • Changelog. (Add an entry to docs/changelog.rst near the top of the document.)
  • Tests. (Encouraged but not strictly required.)

Depending on whether this feature is desired, I'll address the remaining points.

@arsaboo
Copy link
Contributor

arsaboo commented Jul 12, 2023

Just trying to understand...how is this different than beet modify?

@sampsyo
Copy link
Member

sampsyo commented Jul 12, 2023

Very interesting; thanks for getting this started! In addition to @arsaboo's question, I'd also be interested in any thoughts you might have about what it would like to make this a new feature for the existing rewrite plugin instead of a new plugin.

@Maxr1998
Copy link
Contributor Author

Thanks for the quick feedback!

The problem with beet modify for me personally is that I'd have to run it manually every time, and you'd have to redo your modifications every time after running mbsync. That's a problem since moving files around causes my media server, Jellyfin, newly indexes the tracks and loses metadata and information like likes.
Instead, I'd prefer to specify persistent overrides to be honored by beets.

Integrating the changes to the existing rewrite plugin would definitely be possible, however, the biggest problem I see with that is the config format.
My suggested changes require specifying a matcher query, the fieldname and the replacement string. The current value has a dict instead, which I'm not sure we can modify to suit our needs. Especially if it's about readability as well.

@sampsyo
Copy link
Member

sampsyo commented Jul 14, 2023

Nice; thanks for the extra details! All that makes sense to me. It seems like the next step may be writing some documentation—let us know if you need help with that!

@Maxr1998
Copy link
Contributor Author

Thanks, I'll look into it and will post my draft when it's ready, I'd definitely appreciate your feedback then!

@Maxr1998
Copy link
Contributor Author

Maxr1998 commented Sep 30, 2023

Finally got around to writing some documentation. Please let me know if there's anything to add.

Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this all looks good! I found a super minor typo in the new docs, but other than this (and the linter approving; I think it may have some style suggestions), I think this is ready to merge.

docs/plugins/advancedrewrite.rst Outdated Show resolved Hide resolved
@Maxr1998
Copy link
Contributor Author

Maxr1998 commented Oct 2, 2023

Thanks! Fixed the typo and (hopefully) most of the lint warnings.

For the __init__ function, I don't think a docstring is necessary, it's usually missing for other plugins as well.

@Serene-Arc
Copy link
Contributor

@Maxr1998 If you could fix the issues causing the linting errors, you should be good to merge.

@Maxr1998
Copy link
Contributor Author

Sure. I'll have to find out how I can suppress the remaining documentation issues then.

@Serene-Arc
Copy link
Contributor

Don't suppress them, just fix them. They give clear instructions on what to do.

This plugin allows rewriting fields based on a given library query. This can be helpful, for example, when an artist was renamed but you'd like to keep their older releases under their old name, or if you have a single track from a Various Artists release and want to have it included with the original artist.
@Maxr1998
Copy link
Contributor Author

Curiously, most if not all other plugins suppress all pydoc warnings. I assumed that was an accepted practice. Nonetheless, I fixed the remaining linter warnings. The PR should be mergeable now.

@Serene-Arc
Copy link
Contributor

Thank you. We're starting to enforce linting and formatting more rigorously so there's no reason not to start now. Thanks for fixing them.

@Serene-Arc Serene-Arc merged commit 91277a1 into beetbox:master Oct 15, 2023
14 checks passed
@Maxr1998 Maxr1998 deleted the advancedrewrite branch October 15, 2023 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants