Skip to content

Centralise field type defs#6550

Merged
snejus merged 3 commits intomasterfrom
centralise-field-type-defs
Apr 19, 2026
Merged

Centralise field type defs#6550
snejus merged 3 commits intomasterfrom
centralise-field-type-defs

Conversation

@snejus
Copy link
Copy Markdown
Member

@snejus snejus commented Apr 19, 2026

Centralise Field Type Definitions

Summary

This PR eliminates duplicated field-to-type mappings across Album and Item model classes. Previously, each model class maintained its own _fields: dict[str, types.Type] — a verbose, hand-written dictionary that encoded both which fields exist and what type each one has. This duplication made it easy for the two definitions to drift out of sync.


Architecture Change

Before: Each model class owned a full inline _fields dict mapping field names to type instances.

Album._fields = {"id": types.PRIMARY_ID, "artpath": types.NullPathType(), "added": types.DATE, ...}  # ~40 entries
Item._fields  = {"id": types.PRIMARY_ID, "path": types.PathType(), "album_id": types.FOREIGN_ID, ...}  # ~80 entries

After: Field names are declared as a set[str], and the type mapping is derived lazily from a single centralised TYPE_BY_FIELD registry in beets.dbcore.fields.

Album._field_names = {"id", "artpath", "added", ...}        # plain set of names
Item._field_names  = Album._field_names - {"artpath"} | {...} # extends Album's set

LibModel._fields (cached_classproperty) = {f: TYPE_BY_FIELD[f] for f in cls._field_names}

This creates a clean separation of concerns:

  • What fields belong to a model → declared in the model (_field_names)
  • What type a field has → declared once in dbcore.fields.TYPE_BY_FIELD

Key Impacts

Area Before After
Type definitions Inline per model Single registry in dbcore.fields
Item._fields Fully independent dict Derived from Album._field_names
Album.item_keys Hard-coded list of 40+ strings _field_names - {"artpath", "id"}
_media_fields / _media_tag_fields Intersected with _fields.keys() Intersected with _field_names directly
Coverage measurement --cov=beets --cov=beetsplug --cov=. (fixes missing beetsplug/musicbrainz.py)

Risk Areas to Review

  • TYPE_BY_FIELD completeness: all field names in _field_names must have a corresponding entry in the registry — a missing key will raise at class definition time via cached_classproperty.
  • Album.item_keys semantics: changed from list to set and is now derived automatically. Any code that relied on ordering or specific list membership should be checked.
  • Item._field_names inheritance expression: Album._field_names - {"artpath"} | {...} — operator precedence here is (Album._field_names - {"artpath"}) | {...}, which is the intended behaviour, but worth a close read.

Copilot AI review requested due to automatic review settings April 19, 2026 02:43
@snejus snejus requested a review from a team as a code owner April 19, 2026 02:43
@github-actions
Copy link
Copy Markdown

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.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 19, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.78%. Comparing base (82384da) to head (e2e8e80).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6550      +/-   ##
==========================================
+ Coverage   71.19%   71.78%   +0.59%     
==========================================
  Files         150      156       +6     
  Lines       19175    20176    +1001     
  Branches     3084     3214     +130     
==========================================
+ Hits        13651    14484     +833     
- Misses       4864     5006     +142     
- Partials      660      686      +26     
Files with missing lines Coverage Δ
beets/library/fields.py 100.00% <100.00%> (ø)
beets/library/models.py 87.01% <100.00%> (+0.10%) ⬆️

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@snejus snejus force-pushed the centralise-field-type-defs branch 2 times, most recently from 09e61e7 to bb7c0ba Compare April 19, 2026 02:46
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

grug see PR want one place for field -> type mapping, so Album and Item no drift.

Changes:

  • move Album/Item fixed field list to _field_names sets, and derive _fields from shared registry
  • update media-field set logic to use _field_names, and derive Album.item_keys from field set math
  • change coverage task to use --cov=.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
pyproject.toml change pytest-cov source selection for coverage runs
beets/library/models.py replace per-model _fields dicts with _field_names + derived _fields, adjust related logic

Comment thread beets/library/models.py Outdated
Comment thread pyproject.toml
Comment thread beets/library/models.py
Comment thread beets/library/models.py Outdated
@snejus snejus force-pushed the centralise-field-type-defs branch 3 times, most recently from e581fc1 to e3315bd Compare April 19, 2026 03:24
@snejus snejus requested a review from Copilot April 19, 2026 03:25
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comment thread beets/library/models.py
Comment thread beets/library/fields.py
Comment thread beets/library/fields.py
@snejus snejus force-pushed the centralise-field-type-defs branch from e3315bd to ded00cf Compare April 19, 2026 14:02
@snejus snejus requested a review from semohr April 19, 2026 14:02
@snejus snejus force-pushed the centralise-field-type-defs branch from ded00cf to f2d324f Compare April 19, 2026 14:08
snejus added 3 commits April 19, 2026 15:12
I found that beetsplug/musicbrainz.py does not appear under https://app.codecov.io/gh/beetbox/beets/tree/master/beetsplug.

Setting coverage source as repo root should fix this.
@snejus snejus force-pushed the centralise-field-type-defs branch from f2d324f to e2e8e80 Compare April 19, 2026 14:12
@snejus snejus merged commit e3e8793 into master Apr 19, 2026
22 checks passed
@snejus snejus deleted the centralise-field-type-defs branch April 19, 2026 15:32
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.

3 participants