Unified search string construction between albums and items.

[semohr]

Description

Matching albums and singletons has slightly different behavior between singletons and albums.

Albums: When a user does not specify any search query in the manual prompt, the system defaults to using the corresponding value from the metadata. Singletons: Overwrite metadata by any given noneempty search query.

Additionally I noticed that we have an unintentional type error in the metadata_plugins.candidates call. Since *arg and **kwargs are not typed they accepted artist and title/album of type str|None while they should only accept str.

Changes

This PR aligns both methods and uses a common function to parse the search terms. This intentionally couples these two occasions to hopefully prevent drift in the future and makes clear that we use the same logic in these two places.

Added explicit typehints to candidates and item_candidates function.

[codecov[bot]]

Codecov Report

:x: Patch coverage is 78.94737% with 4 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 67.47%. Comparing base (88ca0ce) to head (33d1a35). :white_check_mark: All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
beets/autotag/match.py	86.66%	0 Missing and 2 partials :warning:
beets/metadata_plugins.py	50.00%	2 Missing :warning:

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #6117   +/-   ##
=======================================
  Coverage   67.46%   67.47%           
=======================================
  Files         136      136           
  Lines       18532    18534    +2     
  Branches     3130     3130           
=======================================
+ Hits        12503    12506    +3     
  Misses       5364     5364           
+ Partials      665      664    -1     

Files with missing lines	Coverage Δ
beets/autotag/match.py	77.84% <86.66%> (+0.92%)	:arrow_up:
beets/metadata_plugins.py	76.52% <50.00%> (ø)

:rocket: New features to boost your workflow:

• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[semohr]

@snejus How do we want to continue here?

We could remove the _parse_search_terms_with_fallbacks function and revert to an inline approach, but I’m a bit concerned that an inline solution might drift out of sync again over time. I’d also prefer to keep the new tests, since they effectively serve as a contract for the search-parsing behavior.

That said, in the grand scheme I don't care too much as long as the tests remain in place in some form.

[snejus]

I think I'm missing something... Is this condition so complex it needs testing? https://github.com/beetbox/beets/pull/6117#discussion_r2454370978

[semohr]

I mean it kinda is, otherwise we would not have had this issue here (see also). It is not really about the complexity but ensuring the implementation is well defined and stays as expected, we would use the tests here to ensure the functional requirements.

[snejus]

Remember the reason I did the investigation behind that comment: after your adjustment, I could not any more understand what this code does. The test does not help.

On the positive side, I'm happy that you drew attention to it, because it does not work the way I'd expect it to, and thus we need to adjust it:

-        if not (search_artist and search_name):
+        if not search_artist and not search_album:
            search_artist, search_name = cur_artist, cur_album

...

-    search_artist = search_artist or item.artist
-    search_title = search_title or item.title
+    if not search_artist and not search_title:
+        search_artist, search_title = item.artist, item.title

I think we are on the same page regarding this simple conditional being unambigous and not needing testing?

---

I would be keen to keep it simple and stupid and finally get it merged.

[semohr]

The simple and stupid approach has introduced this issue and has allowed the desync to be introduced in the first place. I would like this a bit more controlled, either have the implementations coupled with a private function and/or have this properly tested. As you are against a private function to couple the item and album logic (which tbh I don't understand, as we pretty much do the same for the _sort_candidate function), we should at-least enforce the behavior by testing it.