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

Perform a regex substitution in the substitute plugin #5357

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

nichobi
Copy link
Contributor

@nichobi nichobi commented Jul 10, 2024

Description

This utilises regex substitution in the substitute plugin. The previous approach only used regex to match the pattern, then replaced it with a static string. This change allows more complex substitutions, where the output depends on the input.

Example use case

Say we want to keep only the first artist of a multi-artist credit, as in the following list:

Neil Young & Crazy Horse -> Neil Young
Michael Hurley, The Holy Modal Rounders, Jeffrey Frederick & The Clamtones -> Michael Hurley
James Yorkston and the Athletes -> James Yorkston

This would previously have required three separate rules, one for each resulting artist. By using a regex substitution, we can get the desired behaviour in a single rule:

substitute:
  ^(.*?)(,| &| and).*: \1

(Capture the text until the first , & or and, then use that capture group as the output)

Notes

I've kept the previous behaviour of only applying the first matching rule, but I'm not 100% sure it's the ideal approach.
I can imagine both cases where you want to apply several rules in sequence and cases where you want to stop after the first match.

To Do

  • Documentation. (If you've added a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
  • Changelog. (Add an entry to docs/changelog.rst to the bottom of one of the lists near the top of the document.)
  • Tests. (Very much encouraged but not strictly required.)

Copy link

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.

@nichobi nichobi changed the title Use a regex substitue in the substitute plugin Use a regex substitute in the substitute plugin Jul 10, 2024
@nichobi nichobi marked this pull request as ready for review July 11, 2024 12:48
@nichobi nichobi changed the title Use a regex substitute in the substitute plugin Perform a regex substitution in the substitute plugin Jul 11, 2024
@Serene-Arc
Copy link
Contributor

Hi! Thanks for the PR. Are you comfortable adding tests for this enhancement? Anything with regex should be heavily tested to make sure it behaves exactly as expected.

@nichobi
Copy link
Contributor Author

nichobi commented Sep 29, 2024

Hi! Thanks for the PR. Are you comfortable adding tests for this enhancement? Anything with regex should be heavily tested to make sure it behaves exactly as expected.

Thanks for the feedback, and sorry it's taken me so long to respond. I've added some testing now, although I've intentionally left one failing.

I had assumed that the order of properties would be preserved when loading the configuration, but this does not seem to be the case. This means it's quite unpredictable what rule will be triggered if the input matches several rules. This would also be the case in the current version of the plugin.

Perhaps the configuration should be refactored into a list of rules, rather than an object, so order is preserved?

Maybe you have thoughts, as the creator of the plugin @fdaniele85?

@fdaniele85
Copy link
Contributor

Hi! I agree that it should be a list of rules and order should be preserved. I never had an overlap of two rules, therefore I did not think about this case.

@nichobi
Copy link
Contributor Author

nichobi commented Oct 5, 2024

I've updated the configuration to use a list of rules, while deprecating the previous syntax.
The old syntax will still work, but with a warning.

Old syntax:

substitute:
  a: b
  c: d

New syntax:

substitute:
- a: b
- c: d

Copy link
Member

@snejus snejus left a comment

Choose a reason for hiding this comment

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

Added several comments

beetsplug/substitute.py Outdated Show resolved Hide resolved
docs/plugins/substitute.rst Outdated Show resolved Hide resolved
test/plugins/test_substitute.py Outdated Show resolved Hide resolved
test/plugins/test_substitute.py Outdated Show resolved Hide resolved
beetsplug/substitute.py Outdated Show resolved Hide resolved
beetsplug/substitute.py Outdated Show resolved Hide resolved
beetsplug/substitute.py Outdated Show resolved Hide resolved
beetsplug/substitute.py Outdated Show resolved Hide resolved
beetsplug/substitute.py Outdated Show resolved Hide resolved
Copy link
Member

@snejus snejus left a comment

Choose a reason for hiding this comment

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

This looks great!!

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.

4 participants