beets icon indicating copy to clipboard operation
beets copied to clipboard

Introduce Info.name property and add types to match details functions

Open snejus opened this issue 2 months ago • 6 comments

Generalize some of common tagging functionality to Info and Match base classes

This PR centralises some common tagging functionality between singletons and albums allowing to simplify ChangeRepresentation and importing functionality. This is prep work for a larger PR which refactors and simplifies the entire tagging workflow.

Changes

  • Core type changes: Changed mapping parameter from Mapping[Item, TrackInfo] to list[tuple[Item, TrackInfo]] in apply_metadata(), distance(), and assign_items() functions
  • Match dataclasses: Converted AlbumMatch and TrackMatch from NamedTuple to @dataclass, introducing base Match class with common functionality
  • New properties: Added name cached property to Info class for unified name access
  • ChangeRepresentation refactor: Converted to @dataclass with lazy property evaluation, replacing cur_album/cur_title with unified cur_name field
  • UI improvements: Simplified display logic by using match.info.name instead of type-specific field checks
  • Parameter renaming: Renamed search_album/search_title parameters to search_name for consistency across singleton and album workflows

The changes maintain backward compatibility in behavior while improving type safety and code clarity.

snejus avatar Nov 02 '25 23:11 snejus

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.

github-actions[bot] avatar Nov 02 '25 23:11 github-actions[bot]

Codecov Report

:x: Patch coverage is 95.79832% with 5 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 68.26%. Comparing base (b058218) to head (60b4a38). :warning: Report is 6 commits behind head on master. :white_check_mark: All tests successful. No failed tests found.

Files with missing lines Patch % Lines
beetsplug/bpsync.py 0.00% 2 Missing :warning:
beetsplug/mbsync.py 60.00% 2 Missing :warning:
beets/autotag/match.py 87.50% 0 Missing and 1 partial :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6142      +/-   ##
==========================================
+ Coverage   68.22%   68.26%   +0.03%     
==========================================
  Files         138      138              
  Lines       18772    18791      +19     
  Branches     3170     3167       -3     
==========================================
+ Hits        12808    12827      +19     
  Misses       5290     5290              
  Partials      674      674              
Files with missing lines Coverage Δ
beets/autotag/__init__.py 87.61% <100.00%> (ø)
beets/autotag/distance.py 82.59% <100.00%> (ø)
beets/autotag/hooks.py 100.00% <100.00%> (ø)
beets/importer/tasks.py 90.89% <100.00%> (ø)
beets/ui/commands/import_/display.py 81.85% <100.00%> (-0.27%) :arrow_down:
beets/ui/commands/import_/session.py 58.62% <100.00%> (ø)
beetsplug/mbpseudo.py 79.87% <100.00%> (-0.13%) :arrow_down:
beets/autotag/match.py 76.92% <87.50%> (ø)
beetsplug/bpsync.py 18.07% <0.00%> (ø)
beetsplug/mbsync.py 81.81% <60.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.

codecov[bot] avatar Nov 02 '25 23:11 codecov[bot]

Just had a short look and this will absolutly break beets-flask downstream. We use the mappings dict quite a bit... Can we move this change in particular to a different PR?

I think the other things are mainly no brainers.

I need to think abit about the implications a how to migrate this in beets-flask. Just a bit more time here would be highly appreciated :)

semohr avatar Nov 03 '25 11:11 semohr

Just had a short look and this will absolutly break beets-flask downstream. We use the mappings dict quite a bit... Can we move this change in particular to a different PR?

I think the other things are mainly no brainers.

I need to think abit about the implications a how to migrate this in beets-flask. Just a bit more time here would be highly appreciated :)

No worries - I kept Match.mapping the same now - instead, I introduced item_info_pairs property.

snejus avatar Nov 12 '25 03:11 snejus

Renaming does not really help me here :laughing:

Where is the usecase for list[tuple[Item, TrackInfo]] over Mapping[Item, TrackInfo]? I feel like the later is cleaner if there is not a specific edgecase I'm missing.

semohr avatar Nov 12 '25 10:11 semohr

Renaming does not really help me here 😆

I'm not sure that I understand - AlbumMatch.mapping is now present like it was before. I just introduce a new property item_info_pairs:

@dataclass
class AlbumMatch(Match):
    info: AlbumInfo
    mapping: dict[Item, TrackInfo]
    extra_items: list[Item]
    extra_tracks: list[TrackInfo]

    @property
    def item_info_pairs(self) -> list[tuple[Item, TrackInfo]]:
        return list(self.mapping.items())

Where is the usecase for list[tuple[Item, TrackInfo]] over Mapping[Item, TrackInfo]? I feel like the later is cleaner if there is not a specific edgecase I'm missing.

It's just a wrong data type for how this property is used across the codebase - it's used for recording and looping across matching (item, track_info) pairs for the album. No functionality in the codebase makes use of the mapping nature of it - instead, values are accessed as mapping.items() which just gets the underlying list of (item, track_info) pairs.

I thus clarified it with the new property.

snejus avatar Nov 15 '25 15:11 snejus

Is there anything missing here @semohr?

snejus avatar Dec 16 '25 22:12 snejus

Btw I really appreciate that you started to properly type some parts in the commands submodule. I was hopeing someone would start with this after the bigger refactor :P

semohr avatar Dec 17 '25 22:12 semohr