Commit e3e8793
authored
Centralise field type defs (#6550)
### 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.4 files changed
+215
-194
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
0 commit comments