support replacing in list-type fields

[xe2io]

This PR addresses #7 by adding support for replacing in the list-type fields returned by the Musicbrainz database.

If the field being modified is a list, the replacements are applied to each item in the list. This has the potential to (further) increase the processing time when importing based on the number of replacement rules, although pre-compiling and reusing the patterns should help.

[xe2io]

Please let me know if you'd like documentation changes and/or additional test cases or fields in the tests. Thanks!

[edgars-supe]

Thanks for the work, looks good to me! I just have a question and a suggestion for you to look at.

This has the potential to (further) increase the processing time when importing based on the number of replacement rules

I'm not too worried about that, I think the README addresses the possible performance penalty when running too many replacements.

Please let me know if you'd like documentation changes and/or additional test cases or fields in the tests. Thanks!

I think you're all set!

[edgars-supe]

Is this syntax compatible with <3.10? Currently the setup.py for this plugin indicates it supports beets >=1.4.7, and I think that version supports Python 3.6. So I'd like to keep it compatible with older versions as well.

[xe2io]

It is not; the X|Y shorthand for Union[X, Y] was added in 3.10 with PEP 604

[edgars-supe]

I think lambda item: self._replace_field(item, replacements) would be nicer, since it'd keep the same logic as the str case.

[snejus]

I think this may be a nice application for @singledispatchmethod, something like

    @singledispatchmethod
    @classmethod
    def _replace(cls, value: Any, replacement: Replacement) -> Any:
        raise NotImplementedError

    @_replace.register(str)
    @classmethod
    def _replace_str(cls, value: str, replacement: Replacement) -> str:
        return replacement[0].sub(repl=replacement[1], string=value)

    @_replace.register(list)
    @classmethod
    def _replace_list(cls, value: list[str], replacement: Replacement) -> list[str]:
        return [cls._replace(item, replacement) for item in value]

[xe2io]

It looks like @singledispatchmethod was introduced in 3.4.

@edgars-supe, would you have an opinion on the minimum version of Python we would want to support?

I appreciate all of these comments and suggestions, I am learning a lot :)

[xe2io]

Thanks for the feedback and suggestions! Backward compatibility wasn't something I had considered (hooray sunny day case!), but will definitely be looking into it. I anticipate it taking a little bit of time to test the different permutations to satisfy my curiosity:
beets 1.4.7 was released in May 2018, 1.4.8 added official support for 3.7 and 3.8 in May 2019, and it wasn't until 1.6.0 in Nov 2021 that 2.7, 3.4, 3.5 support was removed.

This means that Python 3.6 was the current version at time of beets 1.4.7 release and 2.7, 3.4, and 3.5 had not met their end-of-life and were supported.

I'm now extremely curious and (un)?lucky for me there are pre-built container images available for trying it out. At a minimum, definitely with 3.6/1.4.7 and 3.7/2.0.0 (latest beets minimum Python version).

[xe2io]

A slight complication- the list-like fields was only added in beets v2.0.0: beetbox/beets@f72261e
The plugin code which handles this case should be backward-compatible, but the test case helpers will need to handle this (to avoid unexpected keyword args to TrackInfo and AlbumInfo constructors :))

Also- apparently re.Pattern was only added in Python 3.7, which I'll see if I can get the code to handle.

[xe2io]

Some updates:
I've been testing with Python 3.4 (earliest 3.x mentioned by Beets), 3.9 (earliest non-EOL) with beets 1.4.7, 1.4.8, and 1.5.0. There two issues affecting backward-compatibility which I haven't solved yet:

• 1.4.7 fails to register listeners (ValueError: Function has keyword-only arguments or annotations, use getfullargspec() API which can support them) which is fixed in 1.4.8
• <1.5.0 raises a TypeError: argument of type 'AlbumInfo' is not iterable (line 63) (base class changed in 1.5.0)

If we can set the minimum supported Python version to 3.4 (or even 3.9), we could use the @singledispatchmethod to handle both the type-checking anti-pattern and potentially the 2nd point above. I'm not confident that the first can be solved, which would mean the minimum supported beets version would need to be raised to 1.4.8.

As always, any pointers (references? 😁) or ideas are most welcome.

[edgars-supe]

Apologies for my late reply. And thanks for the extensive research, much appreciated!

I'll preface all of this by saying I'm not a Python dev, I don't use Python in my day-to-day, so I don't really have an opinion (or knowledge) on most of these things, but I'll do my best to look into things and make some decisions.

So, as for the Python version, I think it doesn't make sense to support <=3.6, since it is EOL. And since the plugin uses re.Pattern, I'll set the minimum Python version to 3.7. That will also cover @singledispatchmethod.

As for beets - I'll set the minimum version to 1.5.0, as it was released 3 years ago, officially supports Python 3.7 and will cover the two issues you mentioned.

[edgars-supe]

Changes have been pushed to master. You can remove the compatibility things you've added. :)

[snejus]

Note that all Python versions up to and including 3.8 are now EOL. Though, as @edgars-supe mentioned

As for beets - I'll set the minimum version to 1.5.0, as it was released 3 years ago, officially supports Python 3.7 and will cover the two issues you mentioned.

I think it's a good idea to start with picking the oldest beets version to support - this will decide your Python constraint (actually you have little control here 😅).

[snejus]

Also @edgars-supe happy to see that your pick of beets==1.5.0 aligns with what I have in beetcamp: https://github.com/snejus/beetcamp/blob/b356bf6379ff887d78a281e88f5fc32f93287e3e/pyproject.toml#L30