Background
The `_legalize_stage` function was causing issues with Mypy due to
inconsistent type usage between the `path` and `extension` parameters.
This inconsistency stemmed from the `fragment` parameter influencing the
types of these variables.
Key issues
1. `path` was defined as `str`, while `extension` was `bytes`.
2. Depending on `fragment`, `extension` could be either `str` or `bytes`.
3. `path` was sometimes converted to `bytes` within `_legalize_stage`.
Item.destination` method
- The `fragment` parameter determined the output format:
- `False`: Returned absolute path as bytes (default)
- `True`: Returned path relative to library directory as str
Thus
- Rename `fragment` parameter to `relative_to_libdir` for clarity
- Ensure `Item.destination` returns `bytes` in all cases
- Code expecting strings now converts the output to `str`
- Use only `str` type in `_legalize_stage` and `_legalize_path`
functions
- These functions are no longer dependent on `relative_to_libdir`
# Remove Python 2 compatibility code for string encoding/decoding
This PR simplifies the codebase by removing Python 2 compatibility code
related to string encoding and decoding operations. Key changes:
- Remove custom `_convert_args`, `arg_encoding()`, `_fsencoding()` and
`convert_command_args()` functions
- Replace with standard library's `os.fsencode()` and `os.fsdecode()`
for file system path handling
- Simplify bytestring/string conversions throughout the codebase
- Update test code to use modern string handling
These changes reduce code complexity and maintenance burden since Python
2 support is no longer needed.
which added some mypy config to pyproject.toml, leading to mypy ignoring
setup.cfg
(this shows up in CI output for #5701, but is not very visible
since we currently ignore mypy errors)
Related: https://github.com/beetbox/beets/pull/5728
## Description
This might be a quick one, depending on how you feel about it... It
allows you to pickle DB model objects. I don't think this is used
directly in Beets, but it might be useful in general. For instance, we
encountered an issue where we wanted to quickly pickle an Item or Album.
This sometimes worked and other times failed, which seemed quite
inconsistent.
Some DB model methods and properties have the side effect of attaching
an SQLite connection to self (._db), which prevents serialization. The
fix is quite straightforward, so I thought we might want to integrate
this into beets directly.
## To Do
- [x] Changelog
- [x] Tests
some work following up my review of https://github.com/beetbox/beets/pull/5718
- add typings
- refactor the logic in the core `ft_in_title` function, motivated by the
fact that I found this more difficult to read than necessary during the
review
FtInTitle performs a library store operation for every item it
processes, whether or not the item has changed. By limiting the
`item.store()` call to only those cases when the item has changed, the
plugin’s performance when processing an entire library improves by two
to three orders of magnitude.
This PR adds support for alternate metadata backends in the `mbsync` and
`missing` plugins, allowing them to work with any metadata backend the
items are tagged with instead of being limited to just MusicBrainz.
Key changes:
- Removed redundant `albums_for_id` and `items_for_id` methods.
- Updated `album_for_id` and `track_for_id` implementations to return a
single album/track as their names suggest.
- Updated both plugins to use the above functions instead of
MusicBrainz-specific ones.
- Updated `missing` plugin docs to make it clear that missing albums for
artists can only be obtained for the `musicbrainz` metadata backend
- This is possible with other backends but implementation was outside of
this PR's scope.
- Slightly simplified the existing implementation
The changes maintain backwards compatibility while enabling users to
leverage metadata from sources like Deezer, Discogs or Bandcamp when
syncing their libraries.
**Note**: this change came about because I'm working on making
`musicbrainz` a separate plugin: this will be submitted in the next PR.
Using the correct function signature for g_file_new_for_path fixes the
tests on s390x.
I do not have the full story on why this failed consistently only on
s390x, but I guess the big endian might have something to play with
this.
Here is how the tests were failing:
```
169s ___________________________ ThumbnailsTest.test_uri ____________________________
169s
169s self = <test.plugins.test_thumbnails.ThumbnailsTest testMethod=test_uri>
169s
169s def test_uri(self):
169s gio = GioURI()
169s if not gio.available:
169s self.skipTest("GIO library not found")
169s
169s > assert gio.uri("/foo") == "file:///" # silent fail
169s E AssertionError: assert '' == 'file:///'
169s E
169s E - file:///
169s
169s test/plugins/test_thumbnails.py:268: AssertionError
```
You can see a full log here [1] and a history of consistent failure here
[2]. Both links are bound to expire at some point, sorry future
archeologist 🤷.
[1]:
https://autopkgtest.ubuntu.com/results/autopkgtest-plucky/plucky/s390x/b/beets/20250403_162414_5d1da@/log.gz#S5
[2]: https://autopkgtest.ubuntu.com/packages/beets/plucky/s390x
Using the correct function signature for g_file_new_for_path fixes the
tests on s390x.
I do not have the full story on why this failed consistently only on
s390x, but I guess the big endian might have something to play with
this.
Here is how the tests were failing:
```
169s ___________________________ ThumbnailsTest.test_uri ____________________________
169s
169s self = <test.plugins.test_thumbnails.ThumbnailsTest testMethod=test_uri>
169s
169s def test_uri(self):
169s gio = GioURI()
169s if not gio.available:
169s self.skipTest("GIO library not found")
169s
169s > assert gio.uri("/foo") == "file:///" # silent fail
169s E AssertionError: assert '' == 'file:///'
169s E
169s E - file:///
169s
169s test/plugins/test_thumbnails.py:268: AssertionError
```
You can see a full log here [1] and a history of consistent failure
here [2]. Both links are bound to expire at some point, sorry future
archeologist 🤷.
[1]: https://autopkgtest.ubuntu.com/results/autopkgtest-plucky/plucky/s390x/b/beets/20250403_162414_5d1da@/log.gz#S5
[2]: https://autopkgtest.ubuntu.com/packages/beets/plucky/s390x
Fixes#5649, more precisely the behavior described in comment
https://github.com/beetbox/beets/issues/5649#issuecomment-2732245358
where a last.fm genre was found for the album but `_resolve_genre()`
kicked it out because it was't allowed by the whitelist which resulted
in writing an empty genre as the final result.
- This fix makes sure that when no album genre is found the next stage
is hit - returning with either the original genre (if any), the
configured fallback genre or None.
- A new option `--debug` or configuration setting `extended_debug: yes`
can be set to see precisely what tags were fetched during the track,
album or artist stages prior to filtering by a whitelist or
canonicalization tree. It's not enabled by default since it clutters up
the debug log quite a bit especially during *Various Artists* checks,
trying to get the most popular track genre.
Some refactorings to make the plugins behavior better readable and also
fixing some regressions:
- `_resolve_genres` returns a list and so does `_combine_genres` (which
is now renamed to `combine_resolve_and_log` which more precisely
reflects what it's doing).
- _`to_delimited_genre_string` included reducing to count *prevsiously*,
which was kind of hidden in there and not obvious on first sight and
also it did some formatting. The name was bad! It is now called
`format_and_stringify` (the count reduction part moved to
`_resolve_genres`)
- Since `_resolve_genre` does count-reduction when canonicalization is
configured, it makes sense to also do that when only whitelist checks
are done.
- New log message introduced in `_combine_resolve_and_log`showing the
list of existing genres that are taken into account before
whitelist/canonicalization does its magic.
- `_resolve_genre` got a docstring describing what it's doing
- New helper `_filter_valid_genres` now used in `_resolve_genre`
(instead of hardcoded is-valid-list-comp) and other places
(`fetch_genre`)
- `fetch_genre` now also does a valid check. That way it is assured that
if everything was kicked, the next stage is entered (eg. album ->
artist)
- This fallback to next stage is now tested with a new `test_get_genre`
test-case.
This was not thought through clearly before. It now behaves as follows
which I suppose is least surprising to a user:
- force is on, keep_existing is on, but the whitelist is DISABLED
- no stage found anything on last.fm
- fall back to the original genre
If in this example the whitelist would be ENABLED, the behaviour
changes: Only if the existing genre passes the whitelist test the
original is kept.
If no album was found the next stage (artist) should be entered, the
original genre kicked out (not whitelisted) and artist genre accepted.
The log message is slightly misleading since it tried to keep existing
genres but they were not whitelisted, thus kicked out. This is expected.
- Revert/fix last.fm fetcher methods to validate genres.
- In past versions (<=2.2) _resolve_genres which included whitelist
checks ran instantly after fetching last.fm tags which made sure the
next stage is hit when nothing worthwhile was found (e.g fallback
album -> artist).
- Bring back this behavior but don't run a full _resolve_genres but a
quick valid (whitelist) check only!
- Introduce an extended config/CLI option that allows to really log what
each stage fetches (prior to validation/whitelist filtering).
- Since this potentially is verbose especially with VA albums (a lot
of artist tag fetches) for performance and debug log clutter reasons
this is disabled by default.
- Clarify final last.fm tags debug log message to "valid last.fm genres"
