From 1df625cd1bc924301fe7cf807f354cbab458738e Mon Sep 17 00:00:00 2001 From: Jacob Pavlock Date: Mon, 12 Dec 2022 06:57:37 -0800 Subject: [PATCH] refactor!: custom fields now require dictionary access Custom fields must now be accessed via `item.custom["my_custom_field"]` i.e. normal dictionary access. I changed this from normal attribute access as overriding `__getattr__` and `__setattr__` (required for transparent attribute access) had some finicky conflicts with sqlalchemy. Also, it prevented type hinting the custom attribute dictionary as well as integration with data serializers such as pydantic. Overall, the more I used them, the more issues I found, and the more it felt like a hack. I believe the new explicit dictionary access for custom attributes will prove to be more bulletproof. It also explicitly delineates normal and custom attributes which can be useful in some cases. --- .../ab5db9861d30_rename_custom_fields.py | 36 +++++ docs/developers/api/core.rst | 2 +- moe/library/album.py | 125 +++++++-------- moe/library/extra.py | 30 ++-- moe/library/lib_item.py | 30 +--- moe/library/track.py | 57 +++---- moe/plugins/duplicate/dup_cli.py | 39 +++-- moe/plugins/edit/edit_core.py | 4 +- moe/plugins/list.py | 6 +- moe/plugins/musicbrainz/mb_cli.py | 4 +- moe/plugins/musicbrainz/mb_core.py | 40 ++--- moe/query.py | 6 +- tests/conftest.py | 22 +-- tests/library/test_album.py | 32 ++-- tests/library/test_extra.py | 26 +-- tests/library/test_lib_item.py | 150 +++++------------- tests/library/test_track.py | 41 +++-- tests/plugins/add/test_add_core.py | 4 +- tests/plugins/duplicate/test_dup_cli.py | 3 +- tests/plugins/edit/test_edit_core.py | 4 +- tests/plugins/musicbrainz/test_mb_cli.py | 10 +- tests/plugins/musicbrainz/test_mb_core.py | 15 +- tests/plugins/test_list.py | 10 ++ tests/test_query.py | 28 ++-- 24 files changed, 328 insertions(+), 396 deletions(-) create mode 100644 alembic/versions/ab5db9861d30_rename_custom_fields.py diff --git a/alembic/versions/ab5db9861d30_rename_custom_fields.py b/alembic/versions/ab5db9861d30_rename_custom_fields.py new file mode 100644 index 00000000..4922e363 --- /dev/null +++ b/alembic/versions/ab5db9861d30_rename_custom_fields.py @@ -0,0 +1,36 @@ +"""Rename custom fields. + +Revision ID: ab5db9861d30 +Revises: 880c5a2d80ed +Create Date: 2022-12-11 15:24:10.227924 + +""" +from alembic import op + +# revision identifiers, used by Alembic. +revision = "ab5db9861d30" +down_revision = "880c5a2d80ed" +branch_labels = None +depends_on = None + + +def upgrade(): + with op.batch_alter_table("album", schema=None) as batch_op: + batch_op.alter_column("_custom_fields", new_column_name="custom") + + with op.batch_alter_table("extra", schema=None) as batch_op: + batch_op.alter_column("_custom_fields", new_column_name="custom") + + with op.batch_alter_table("track", schema=None) as batch_op: + batch_op.alter_column("_custom_fields", new_column_name="custom") + + +def downgrade(): + with op.batch_alter_table("track", schema=None) as batch_op: + batch_op.alter_column("custom", new_column_name="_custom_fields") + + with op.batch_alter_table("extra", schema=None) as batch_op: + batch_op.alter_column("custom", new_column_name="_custom_fields") + + with op.batch_alter_table("album", schema=None) as batch_op: + batch_op.alter_column("custom", new_column_name="_custom_fields") diff --git a/docs/developers/api/core.rst b/docs/developers/api/core.rst index 14c358ad..dacb238e 100644 --- a/docs/developers/api/core.rst +++ b/docs/developers/api/core.rst @@ -28,7 +28,7 @@ Library .. automodule:: moe.library :members: - :exclude-members: cache_ok, path + :exclude-members: cache_ok, path, custom :show-inheritance: :special-members: __init__ diff --git a/moe/library/album.py b/moe/library/album.py index 820fdaaf..e4f86fdb 100644 --- a/moe/library/album.py +++ b/moe/library/album.py @@ -28,29 +28,6 @@ class Hooks: """Album hook specifications.""" - @staticmethod - @moe.hookspec - def create_custom_album_fields() -> dict[str, Any]: # type: ignore - """Creates new custom fields for an Album. - - Returns: - Dict of the field names to their default values or ``None`` for no default. - - Example: - .. code:: python - - return {"my_new_field": "default value", "other_field": None} - - You can then access your new field as if it were a normal field:: - - album.my_new_field = "awesome new value" - - Important: - Your custom field should follow the same naming rules as any other python - variable i.e. no spaces, starts with a letter, and consists solely of - alpha-numeric and underscore characters. - """ # noqa: DAR202 - @staticmethod @moe.hookspec def is_unique_album(album: "Album", other: "Album") -> bool: # type: ignore @@ -90,6 +67,7 @@ class MetaAlbum(MetaLibItem): catalog_nums (Optional[set[str]]): Set of all catalog numbers. country (Optional[str]): Country the album was released in (two character identifier). + custom (dict[str, Any]): Dictionary of custom fields. date (Optional[datetime.date]): Album release date. disc_total (Optional[int]): Number of discs in the album. label (Optional[str]): Album release label. @@ -119,8 +97,7 @@ def __init__( **kwargs, ): """Creates a MetaAlbum object with any additional custom fields as kwargs.""" - self._custom_fields = self._get_default_custom_fields() - self._custom_fields_set = set(self._custom_fields) + self.custom = kwargs self.artist = artist self.barcode = barcode @@ -137,9 +114,6 @@ def __init__( if not tracks: self.tracks = [] - for key, value in kwargs.items(): - setattr(self, key, value) - if config.CONFIG.settings.original_date and self.original_date: self.date = self.original_date @@ -183,7 +157,7 @@ def fields(self) -> set[str]: "original_date", "title", "track_total", - }.union(self._custom_fields) + } def get_track(self, track_num: int, disc: int = 1) -> Optional["MetaTrack"]: """Gets a MetaTrack by its track number.""" @@ -222,6 +196,13 @@ def merge(self, other: "MetaAlbum", overwrite: bool = False) -> None: if other_value and (overwrite or (not overwrite and not self_value)): setattr(self, field, other_value) + for custom_field in self.custom: + other_value = other.custom.get(custom_field) + if other_value and ( + overwrite or (not overwrite and not self.custom[custom_field]) + ): + self.custom[custom_field] = other_value + log.debug( f"MetaAlbums merged. [album_a={self!r}, album_b={other!r}, {overwrite=!r}]" ) @@ -279,7 +260,7 @@ def __repr__(self): repr_str = "AlbumInfo(" + ", ".join(field_reprs) custom_field_reprs = [] - for custom_field, value in self._custom_fields.items(): + for custom_field, value in self.custom.items(): custom_field_reprs.append(f"{custom_field}={value}") if custom_field_reprs: repr_str += ", custom_fields=[" + ", ".join(custom_field_reprs) + "]" @@ -292,14 +273,6 @@ def __repr__(self): repr_str += ")" return repr_str - def _get_default_custom_fields(self) -> dict[str, Any]: - """Returns the default custom album fields.""" - return { - field: default_val - for plugin_fields in config.CONFIG.pm.hook.create_custom_album_fields() - for field, default_val in plugin_fields.items() - } - # Album generic, used for typing classmethod A = TypeVar("A", bound="Album") @@ -316,6 +289,7 @@ class Album(LibItem, SABase, MetaAlbum): catalog_nums (Optional[set[str]]): Set of all catalog numbers. country (Optional[str]): Country the album was released in (two character identifier). + custom (dict[str, Any]): Dictionary of custom fields. date (datetime.date): Album release date. disc_total (int): Number of discs in the album. extras (list[Extra]): Extra non-track files associated with the album. @@ -347,7 +321,7 @@ class Album(LibItem, SABase, MetaAlbum): ) title: str = cast(str, Column(String, nullable=False)) track_total: Optional[int] = cast(Optional[int], Column(Integer, nullable=True)) - _custom_fields: dict[str, Any] = cast( + custom: dict[str, Any] = cast( dict[str, Any], Column( MutableDict.as_mutable(JSON(none_as_null=True)), @@ -375,30 +349,31 @@ def __init__( artist: str, title: str, date: datetime.date, + barcode: Optional[str] = None, + catalog_nums: Optional[set[str]] = None, + country: Optional[str] = None, disc_total=1, + label: Optional[str] = None, + media: Optional[str] = None, + original_date: Optional[datetime.date] = None, + track_total: Optional[int] = None, **kwargs, ): - """Creates an Album. - - Args: - path: Filesystem path of the album directory. - artist: Album artist. - title: Album title. - date: Album release date. - disc_total: Number of discs in the album. - **kwargs: Any other fields to assign to the album. - """ - self._custom_fields = self._get_default_custom_fields() - self._custom_fields_set = set(self._custom_fields) + """Creates an Album object with any additional custom fields as kwargs.""" + self.custom = kwargs self.path = path self.artist = artist - self.title = title + self.barcode = barcode + self.catalog_nums = catalog_nums + self.country = country self.date = date self.disc_total = disc_total - - for key, value in kwargs.items(): - setattr(self, key, value) + self.label = label + self.media = media + self.original_date = original_date + self.track_total = track_total + self.title = title if config.CONFIG.settings.original_date and self.original_date: self.date = self.original_date @@ -483,6 +458,30 @@ def merge(self, other: Union["Album", MetaAlbum], overwrite: bool = False) -> No """ log.debug(f"Merging albums. [album_a={self!r}, album_b={other!r}") + self._merge_tracks(other, overwrite) + self._merge_extras(other, overwrite) + + for field in self.fields: + other_value = getattr(other, field, None) + self_value = getattr(self, field, None) + if other_value and (overwrite or (not overwrite and not self_value)): + setattr(self, field, other_value) + + for custom_field in self.custom: + other_value = other.custom.get(custom_field) + if other_value and ( + overwrite or (not overwrite and not self.custom[custom_field]) + ): + self.custom[custom_field] = other_value + + log.debug( + f"Albums merged. [album_a={self!r}, album_b={other!r}, {overwrite=!r}]" + ) + + def _merge_tracks( + self, other: Union["Album", MetaAlbum], overwrite: bool = False + ) -> None: + """Merges the tracks of another album into this one.""" new_tracks: list["Track"] = [] for other_track in other.tracks: conflict_track = None @@ -494,6 +493,10 @@ def merge(self, other: Union["Album", MetaAlbum], overwrite: bool = False) -> No new_tracks.append(other_track) self.tracks.extend(new_tracks) + def _merge_extras( + self, other: Union["Album", MetaAlbum], overwrite: bool = False + ) -> None: + """Merges the extras of another album into this one.""" if isinstance(other, Album): new_extras: list["Extra"] = [] for other_extra in other.extras: @@ -504,16 +507,6 @@ def merge(self, other: Union["Album", MetaAlbum], overwrite: bool = False) -> No new_extras.append(other_extra) self.extras.extend(new_extras) - for field in self.fields: - other_value = getattr(other, field, None) - self_value = getattr(self, field, None) - if other_value and (overwrite or (not overwrite and not self_value)): - setattr(self, field, other_value) - - log.debug( - f"Albums merged. [album_a={self!r}, album_b={other!r}, {overwrite=!r}]" - ) - @hybrid_property def original_year(self) -> Optional[int]: # type: ignore """Gets an Album's year.""" @@ -546,7 +539,7 @@ def __repr__(self): repr_str = "Album(" + ", ".join(field_reprs) custom_field_reprs = [] - for custom_field, value in self._custom_fields.items(): + for custom_field, value in self.custom.items(): custom_field_reprs.append(f"{custom_field}={value}") if custom_field_reprs: repr_str += ", custom_fields=[" + ", ".join(custom_field_reprs) + "]" diff --git a/moe/library/extra.py b/moe/library/extra.py index 9e6ccb93..b728de22 100644 --- a/moe/library/extra.py +++ b/moe/library/extra.py @@ -70,12 +70,13 @@ class Extra(LibItem, SABase): Attributes: album (Album): Album the extra file belongs to. + custom (dict[str, Any]): Dictionary of custom fields. path (pathlib.Path): Filesystem path of the extra file. """ __tablename__ = "extra" - _custom_fields: dict[str, Any] = cast( + custom: dict[str, Any] = cast( dict[str, Any], Column( MutableDict.as_mutable(JSON(none_as_null=True)), @@ -93,23 +94,19 @@ def __init__(self, album: Album, path: Path, **kwargs): Args: album: Album the extra file belongs to. path: Filesystem path of the extra file. - **kwargs: Any other fields to assign to the extra. + **kwargs: Any custom fields to assign to the extra. """ - self._custom_fields = self._get_default_custom_fields() - self._custom_fields_set = set(self._custom_fields) + self.custom = kwargs album.extras.append(self) self.path = path - for key, value in kwargs.items(): - setattr(self, key, value) - log.debug(f"Extra created. [extra={self!r}]") @property def fields(self) -> set[str]: """Returns any editable, extra fields.""" - return {"album", "path"}.union(self._custom_fields) + return {"album", "path"} @property def rel_path(self) -> PurePath: @@ -147,6 +144,13 @@ def merge(self, other: "Extra", overwrite: bool = False): if other_value and (overwrite or (not overwrite and not self_value)): setattr(self, field, other_value) + for custom_field in self.custom: + other_value = other.custom.get(custom_field) + if other_value and ( + overwrite or (not overwrite and not self.custom[custom_field]) + ): + self.custom[custom_field] = other_value + log.debug( f"Extras merged. [extra_a={self!r}, extra_b={other!r}, {overwrite=!r}]" ) @@ -181,7 +185,7 @@ def __repr__(self): repr_str = "Extra(" + ", ".join(field_reprs) + f", album='{self.album}'" custom_field_reprs = [] - for custom_field, value in self._custom_fields.items(): + for custom_field, value in self.custom.items(): custom_field_reprs.append(f"{custom_field}={value}") if custom_field_reprs: repr_str += ", custom_fields=[" + ", ".join(custom_field_reprs) + "]" @@ -192,11 +196,3 @@ def __repr__(self): def __str__(self): """String representation of an Extra.""" return f"{self.album}: {self.rel_path}" - - def _get_default_custom_fields(self) -> dict[str, Any]: - """Returns the default custom extra fields.""" - return { - field: default_val - for plugin_fields in config.CONFIG.pm.hook.create_custom_extra_fields() - for field, default_val in plugin_fields.items() - } diff --git a/moe/library/lib_item.py b/moe/library/lib_item.py index 00b5e322..8c1c0fff 100644 --- a/moe/library/lib_item.py +++ b/moe/library/lib_item.py @@ -268,19 +268,7 @@ class MetaLibItem: These objects do not exist on the filesystem nor in the library. """ - _custom_fields = {} - _custom_fields_set = None - - @property - def custom_fields(self) -> set[str]: - """Returns the custom fields of an item.""" - if self._custom_fields_set is None: - object.__setattr__( - self, "_custom_fields_set", set(self._get_default_custom_fields()) - ) - - assert self._custom_fields_set is not None - return self._custom_fields_set + custom = {} def _get_default_custom_fields(self) -> dict[str, Any]: """Returns the default custom fields of an item.""" @@ -291,19 +279,9 @@ def fields(self) -> set[str]: """Returns the editable fields of an item.""" raise NotImplementedError - def __getattr__(self, name: str): - """See if ``name`` is a custom field.""" - if name in self.custom_fields: - return self._custom_fields[name] - else: - raise AttributeError from None - - def __setattr__(self, name, value): - """Set custom custom_fields if a valid key.""" - if name in self.custom_fields: - self._custom_fields[name] = value - else: - super().__setattr__(name, value) + def merge(self, other, overwrite: bool = False) -> None: + """Merges another item into this one.""" + raise NotImplementedError def __lt__(self, other): """Library items implement the `lt` magic method to allow sorting.""" diff --git a/moe/library/track.py b/moe/library/track.py index f25fd809..02225c6b 100644 --- a/moe/library/track.py +++ b/moe/library/track.py @@ -151,6 +151,7 @@ class MetaTrack(MetaLibItem): album (Optional[Album]): Corresponding Album object. artist (Optional[str]) artists (Optional[set[str]]): Set of all artists. + custom (dict[str, Any]): Dictionary of custom fields. disc (Optional[int]): Disc number the track is on. genres (Optional[set[str]]): Set of all genres. title (Optional[str]) @@ -169,8 +170,7 @@ def __init__( **kwargs, ): """Creates a MetaTrack object with any additional custom fields as kwargs.""" - self._custom_fields = self._get_default_custom_fields() - self._custom_fields_set = set(self._custom_fields) + self.custom = kwargs self.album = album album.tracks.append(self) @@ -182,9 +182,6 @@ def __init__( self.genres = genres self.title = title - for key, value in kwargs.items(): - setattr(self, key, value) - log.debug(f"MetaTrack created. [track={self!r}]") @property @@ -218,7 +215,7 @@ def fields(self) -> set[str]: "genres", "title", "track_num", - }.union(set(self._custom_fields)) + } def merge(self, other: "MetaTrack", overwrite: bool = False): """Merges another track into this one. @@ -238,6 +235,13 @@ def merge(self, other: "MetaTrack", overwrite: bool = False): if other_value and (overwrite or (not overwrite and not self_value)): setattr(self, field, other_value) + for custom_field in self.custom: + other_value = other.custom.get(custom_field) + if other_value and ( + overwrite or (not overwrite and not self.custom[custom_field]) + ): + self.custom[custom_field] = other_value + log.debug( f"Tracks merged. [track_a={self!r}, track_b={other!r}, {overwrite=!r}]" ) @@ -279,7 +283,7 @@ def __repr__(self): ) custom_field_reprs = [] - for custom_field, value in self._custom_fields.items(): + for custom_field, value in self.custom.items(): custom_field_reprs.append(f"{custom_field}={value}") if custom_field_reprs: repr_str += ", custom_fields=[" + ", ".join(custom_field_reprs) + "]" @@ -291,14 +295,6 @@ def __str__(self): """String representation of a track.""" return f"{self.artist} - {self.title}" - def _get_default_custom_fields(self) -> dict[str, Any]: - """Returns the default custom track fields.""" - return { - field: default_val - for plugin_fields in config.CONFIG.pm.hook.create_custom_track_fields() - for field, default_val in plugin_fields.items() - } - class Track(LibItem, SABase, MetaTrack): """A single track in the library. @@ -307,6 +303,7 @@ class Track(LibItem, SABase, MetaTrack): album (Album): Corresponding Album object. artist (str) artists (Optional[set[str]]): Set of all artists. + custom (dict[str, Any]): Dictionary of custom fields. disc (int): Disc number the track is on. genres (Optional[set[str]]): Set of all genres. path (Path): Filesystem path of the track file. @@ -330,7 +327,7 @@ class Track(LibItem, SABase, MetaTrack): ) title: str = cast(str, Column(String, nullable=False)) track_num: int = cast(int, Column(Integer, nullable=False)) - _custom_fields: dict[str, Any] = cast( + custom: dict[str, Any] = cast( dict[str, Any], Column( MutableDict.as_mutable(JSON(none_as_null=True)), @@ -350,6 +347,10 @@ def __init__( path: Path, title: str, track_num: int, + artist: Optional[str] = None, + artists: Optional[set[str]] = None, + disc: Optional[int] = None, + genres: Optional[set[str]] = None, **kwargs, ): """Creates a Track. @@ -359,25 +360,25 @@ def __init__( path: Filesystem path of the track file. title: Title of the track. track_num: Track number. - **kwargs: Any other fields to assign to the track. + artist: Track artist. Defaults to the album artist if not given. + artists: Set of all artists. + disc: Disc the track belongs to. If not given, will try to guess the disc + based on the ``path`` of the track. + genres (Optional[set[str]]): Set of all genres. + **kwargs: Any custom fields to assign to the track. """ - self._custom_fields = self._get_default_custom_fields() - self._custom_fields_set = set(self._custom_fields) + self.custom = kwargs album.tracks.append(self) + self.path = path + self.artist = artist or self.album.artist + self.artists = artists + self.disc = disc or self._guess_disc() + self.genres = genres self.title = title self.track_num = track_num - for key, value in kwargs.items(): - setattr(self, key, value) - - if self.artist is None: - self.artist = self.album.artist - - if not self.disc: - self.disc = self._guess_disc() - log.debug(f"Track created. [track={self!r}]") def _guess_disc(self) -> int: diff --git a/moe/plugins/duplicate/dup_cli.py b/moe/plugins/duplicate/dup_cli.py index 0a71d69c..d6ce70de 100644 --- a/moe/plugins/duplicate/dup_cli.py +++ b/moe/plugins/duplicate/dup_cli.py @@ -1,7 +1,7 @@ """Adds a duplicate resolution prompt to the CLI.""" import logging -from typing import Optional, Union, cast +from typing import Any, Optional, Union, cast from rich.columns import Columns from rich.console import Group, RenderableType @@ -13,7 +13,7 @@ import moe import moe.cli from moe.cli import console -from moe.library import Album, Extra, LibItem +from moe.library import Album, Extra, LibItem, Track from moe.plugins.remove import remove_item from moe.util.cli import PromptChoice, choice_prompt @@ -116,6 +116,7 @@ def _fmt_item_text( header = Text(f"{item.rel_path}\n{item.album}", justify="center") omit_fields = {"album", "rel_path", "path"} else: + item = cast(Track, item) header = Text(f"{item.title}\n{item.album}", justify="center") omit_fields = { "album", @@ -128,11 +129,21 @@ def _fmt_item_text( item_diff = Table.grid("field value", padding=(0, 1)) item_has_diff = False for field in sorted(item.fields - omit_fields): - field_diff = _fmt_field_vs(item, other, field) + field_diff = _fmt_value_vs( + getattr(item, field, None), getattr(other, field, None) + ) if field_diff: item_has_diff = True item_diff.add_row(Text(field, style="italic grey69"), field_diff) + for custom_field in sorted(set(item.custom).union(set(other.custom))): + field_diff = _fmt_value_vs( + item.custom.get(custom_field), other.custom.get(custom_field) + ) + if field_diff: + item_has_diff = True + item_diff.add_row(Text(custom_field, style="italic grey69"), field_diff) + if isinstance(item, Album) and isinstance(other, Album): item_diff = Group(item_diff, "", _fmt_album_lists(item, other)) item_has_diff = True @@ -175,13 +186,12 @@ def _fmt_album_lists(album: Album, other: Album) -> Union[Group, Table]: return tracklist -def _fmt_field_vs(item: LibItem, other: LibItem, field: str) -> Optional[Text]: - """Highlights field differences between two items. +def _fmt_value_vs(value_a: Any, value_b: Any) -> Optional[Text]: + """Highlights differences between two values. Args: - item: Item to base comparisons off of. - other: Other item to compare against. - field: Field to compare. + value_a: Value to base comparisons off of. + value_b: Other value to compare against. Returns: A rich 'Text' based on the following cases: @@ -195,14 +205,11 @@ def _fmt_field_vs(item: LibItem, other: LibItem, field: str) -> Optional[Text]: 4. `item.field` exists and `other.field` DNE: `item.field` will be returned in green """ - item_field = getattr(item, field, None) - other_field = getattr(other, field, None) - - if not item_field or (item_field == other_field): + if not value_a or (value_a == value_b): return None - if item_field != other_field: - return Text(str(item_field), style="yellow") - if item_field and not other_field: - return Text(str(item_field), style="green") + if value_a != value_b: + return Text(str(value_a), style="yellow") + if value_a and not value_b: + return Text(str(value_a), style="green") return None diff --git a/moe/plugins/edit/edit_core.py b/moe/plugins/edit/edit_core.py index 657c20ac..fb629506 100644 --- a/moe/plugins/edit/edit_core.py +++ b/moe/plugins/edit/edit_core.py @@ -36,8 +36,8 @@ def edit_item(item: LibItem, field: str, value: str): # noqa: C901 try: attr = getattr(item.__class__, field) except AttributeError as a_err: - if field in item._custom_fields: - setattr(item, field, value) + if field in item.custom: + item.custom[field] = value return raise EditError(f"Invalid field given. [{field=!r}]") from a_err diff --git a/moe/plugins/list.py b/moe/plugins/list.py index 3ccb9af9..bddf945d 100644 --- a/moe/plugins/list.py +++ b/moe/plugins/list.py @@ -153,9 +153,13 @@ def _get_base_dict(item: LibItem) -> dict[str, Any]: Returns a dict representation of an Item in the form { attribute: value }. """ item_dict = {} - for attr in sorted(item.fields): + for attr in item.fields: value = getattr(item, attr) if value: item_dict[attr] = value + for custom_field, value in item.custom.items(): + if value: + item_dict[custom_field] = value + return item_dict diff --git a/moe/plugins/musicbrainz/mb_cli.py b/moe/plugins/musicbrainz/mb_cli.py index 14b9cb86..a5f740da 100644 --- a/moe/plugins/musicbrainz/mb_cli.py +++ b/moe/plugins/musicbrainz/mb_cli.py @@ -64,9 +64,9 @@ def _parse_args(args: argparse.Namespace): for item in items: release_id: Optional[str] = None if isinstance(item, (Extra, Track)): - release_id = item.album.mb_album_id + release_id = item.album.custom.get("mb_album_id") elif isinstance(item, Album): - release_id = item.mb_album_id + release_id = item.custom.get("mb_album_id") if release_id: releases.add(release_id) diff --git a/moe/plugins/musicbrainz/mb_core.py b/moe/plugins/musicbrainz/mb_core.py index a2a8f055..adb69a85 100644 --- a/moe/plugins/musicbrainz/mb_core.py +++ b/moe/plugins/musicbrainz/mb_core.py @@ -103,18 +103,6 @@ def add_config_validator(settings: dynaconf.base.LazySettings): ) -@moe.hookimpl -def create_custom_album_fields() -> dict[str, Any]: # type: ignore - """Adds relevant musicbrainz fields to an album.""" - return {"mb_album_id": None} - - -@moe.hookimpl -def create_custom_track_fields() -> dict[str, Any]: # type: ignore - """Adds relevant musicbrainz fields to a track.""" - return {"mb_track_id": None} - - @moe.hookimpl def get_candidates(album: Album) -> list[CandidateAlbum]: """Applies musicbrainz metadata changes to a given album. @@ -139,8 +127,8 @@ def get_candidates(album: Album) -> list[CandidateAlbum]: search_criteria["catno"] = next(iter(album.catalog_nums)) # get any cat_num if album.label: search_criteria["label"] = album.label - if album.mb_album_id: - search_criteria["reid"] = album.mb_album_id + if album.custom.get("mb_album_id"): + search_criteria["reid"] = album.custom["mb_album_id"] if album.media: search_criteria["format"] = album.media if album.track_total: @@ -168,8 +156,8 @@ def process_removed_items(items: list[LibItem]): mb_ids = [] for item in items: - if isinstance(item, Album) and item.mb_album_id: - mb_ids.append(item.mb_album_id) + if isinstance(item, Album) and item.custom.get("mb_album_id"): + mb_ids.append(item.custom["mb_album_id"]) if mb_ids: try: @@ -186,8 +174,8 @@ def process_new_items(items: list[LibItem]): releases = set() for item in items: - if isinstance(item, Album) and item.mb_album_id: - releases.add(item.mb_album_id) + if isinstance(item, Album) and item.custom.get("mb_album_id"): + releases.add(item.custom["mb_album_id"]) if releases: try: @@ -210,12 +198,14 @@ def read_custom_tags( @moe.hookimpl def sync_metadata(item: LibItem): """Sync musibrainz metadata for associated items.""" - if isinstance(item, Album) and hasattr(item, "mb_album_id"): - item.merge(get_album_by_id(item.mb_album_id), overwrite=True) - elif isinstance(item, Track) and hasattr(item, "mb_track_id"): + if isinstance(item, Album) and item.custom.get("mb_album_id"): + item.merge(get_album_by_id(item.custom["mb_album_id"]), overwrite=True) + elif isinstance(item, Track) and item.custom.get("mb_track_id"): item = cast(Track, item) item.merge( - get_track_by_id(item.mb_track_id, item.album.mb_album_id), + get_track_by_id( + item.custom["mb_track_id"], item.album.custom["mb_album_id"] + ), overwrite=True, ) @@ -225,8 +215,8 @@ def write_custom_tags(track: Track): """Write musicbrainz ID fields as tags.""" audio_file = mediafile.MediaFile(track.path) - audio_file.mb_albumid = track.album.mb_album_id - audio_file.mb_releasetrackid = track.mb_track_id + audio_file.mb_albumid = track.album.custom.get("mb_album_id") + audio_file.mb_releasetrackid = track.custom.get("mb_track_id") audio_file.save() @@ -511,7 +501,7 @@ def get_track_by_id(track_id: str, album_id: str) -> MetaTrack: album = get_album_by_id(album_id) for track in album.tracks: - if track.mb_track_id == track_id: + if track.custom.get("mb_track_id") == track_id: log.info(f"Fetched track from musicbrainz. [{track=!r}]") return track diff --git a/moe/query.py b/moe/query.py index 632abfff..1e0e5251 100644 --- a/moe/query.py +++ b/moe/query.py @@ -234,9 +234,9 @@ def _getattr(item_class: Type[LibItem], field: str): attr = getattr(item_class, field) except AttributeError: # assume custom field - custom_func = sa.func.json_each( - item_class._custom_fields, f"$.{field}" - ).table_valued("value", joins_implicitly=True) + custom_func = sa.func.json_each(item_class.custom, f"$.{field}").table_valued( + "value", joins_implicitly=True + ) return custom_func.c.value diff --git a/tests/conftest.py b/tests/conftest.py index e9cb3c3b..9667619b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -5,7 +5,7 @@ import sys import textwrap from pathlib import Path -from typing import Any, Callable, Iterator, Optional +from typing import Callable, Iterator, Optional from unittest.mock import MagicMock import pytest @@ -151,7 +151,6 @@ def track_factory( album: Optional[Album] = None, exists: bool = False, dup_track: Optional[Track] = None, - custom_fields: Optional[dict[str, Any]] = None, **kwargs, ): """Creates a track. @@ -161,7 +160,6 @@ def track_factory( exists: Whether the track should exist on the filesystem. Note, this option requires the write plugin. dup_track: If given, the new track created will be a duplicate of `dup_track`. - custom_fields: Dict of custom_fields to values to assign to the track. **kwargs: Any other fields to assign to the Track. Returns: @@ -190,11 +188,6 @@ def track_factory( **kwargs, ) - if custom_fields: - for field, value in custom_fields.items(): - track.custom_fields.add(field) - setattr(track, field, value) - if dup_track: for field in dup_track.fields: value = getattr(dup_track, field) @@ -217,7 +210,6 @@ def extra_factory( path: Optional[Path] = None, exists: bool = False, dup_extra: Optional[Extra] = None, - custom_fields: Optional[dict[str, Any]] = None, **kwargs, ) -> Extra: """Creates an extra for testing. @@ -227,7 +219,6 @@ def extra_factory( path: Path to assign to the extra. Will create a random path if not given. exists: Whether the extra should actually exist on the filesystem. dup_extra: If given, the new extra created will be a duplicate of `dup_extra`. - custom_fields: Dict of custom_fields to values to assign to the extra. **kwargs: Any other fields to assign to the extra. Returns: @@ -238,11 +229,6 @@ def extra_factory( extra = Extra(album=album, path=path, **kwargs) - if custom_fields: - for field, value in custom_fields.items(): - extra.custom_fields.add(field) - setattr(extra, field, value) - if dup_extra: for field in dup_extra.fields: value = getattr(dup_extra, field) @@ -264,7 +250,6 @@ def album_factory( num_discs: int = 1, exists: bool = False, dup_album: Optional[Album] = None, - custom_fields: Optional[dict[str, Any]] = None, **kwargs, ) -> Album: """Creates an album. @@ -276,7 +261,6 @@ def album_factory( exists: Whether the album should exist on the filesystem. Note, this option requires the write plugin. dup_album: If given, the new album created will be a duplicate of `dup_album`. - custom_fields: Dict of custom_fields to values to assign to the album. **kwargs: Any other fields to assign to the album. Returns: @@ -298,10 +282,6 @@ def album_factory( track_total=num_tracks, **kwargs, ) - if custom_fields: - for field, value in custom_fields.items(): - album.custom_fields.add(field) - setattr(album, field, value) if dup_album: for field in dup_album.fields: diff --git a/tests/library/test_album.py b/tests/library/test_album.py index a1d5b916..c130b07f 100644 --- a/tests/library/test_album.py +++ b/tests/library/test_album.py @@ -17,12 +17,6 @@ class MyAlbumPlugin: """Plugin that implements the extra hooks for testing.""" - @staticmethod - @moe.hookimpl - def create_custom_album_fields(): - """Create a new custom field.""" - return {"no_default": None, "default": "value"} - @staticmethod @moe.hookimpl def is_unique_album(album, other): @@ -34,14 +28,6 @@ def is_unique_album(album, other): class TestHooks: """Test album hooks.""" - def test_create_custom_fields(self, tmp_config): - """Plugins can define new custom fields.""" - tmp_config(extra_plugins=[ExtraPlugin(MyAlbumPlugin, "album_plugin")]) - album = album_factory() - - assert not album.no_default - assert album.default == "value" - def test_is_unique_album(self, tmp_config): """Plugins can add additional unique constraints.""" tmp_config(extra_plugins=[ExtraPlugin(MyAlbumPlugin, "album_plugin")]) @@ -226,6 +212,15 @@ def test_overwrite_tracks(self): assert overwrite_track.title == "conflict" + def test_custom_fields(self): + """Merge custom fields too.""" + album = MetaAlbum(custom=None) + other_album = MetaAlbum(custom="new") + + album.merge(other_album) + + assert album.custom["custom"] == "new" + class TestAlbumMerge: """Test merging two Albums together.""" @@ -333,6 +328,15 @@ def test_overwrite_tracks(self): assert overwrite_track.title == "conflict" + def test_custom_fields(self): + """Merge custom fields too.""" + album = album_factory(custom="") + other_album = album_factory(custom="new") + + album.merge(other_album) + + assert album.custom["custom"] == "new" + class TestFromDir: """Test a creating an album from a directory.""" diff --git a/tests/library/test_extra.py b/tests/library/test_extra.py index d6260fb0..3632a92e 100644 --- a/tests/library/test_extra.py +++ b/tests/library/test_extra.py @@ -11,12 +11,6 @@ class MyExtraPlugin: """Plugin that implements the extra hooks for testing.""" - @staticmethod - @moe.hookimpl - def create_custom_extra_fields(): - """Create a new custom field.""" - return {"no_default": None, "default": "value"} - @staticmethod @moe.hookimpl def is_unique_extra(extra, other): @@ -28,14 +22,6 @@ def is_unique_extra(extra, other): class TestHooks: """Test extra hooks.""" - def test_create_custom_fields(self, tmp_config): - """Plugins can define new custom fields.""" - tmp_config(extra_plugins=[ExtraPlugin(MyExtraPlugin, "extra_plugin")]) - extra = extra_factory() - - assert not extra.no_default - assert extra.default == "value" - def test_is_unique_extra(self, tmp_config): """Plugins can add additional unique constraints.""" tmp_config(extra_plugins=[ExtraPlugin(MyExtraPlugin, "extra_plugin")]) @@ -77,21 +63,21 @@ def test_conflict_persists(self): def test_merge_non_conflict(self): """Apply any non-conflicting fields.""" - extra = extra_factory(custom_fields={"custom": "keep"}) - other_extra = extra_factory(custom_fields={"custom": None}) + extra = extra_factory(custom="keep") + other_extra = extra_factory(custom=None) extra.merge(other_extra) - assert extra.custom == "keep" + assert extra.custom["custom"] == "keep" def test_none_merge(self): """Don't merge in any null values.""" - extra = extra_factory(custom_fields={"custom": None}) - other_extra = extra_factory(custom_fields={"custom": "keep"}) + extra = extra_factory(custom=None) + other_extra = extra_factory(custom="keep") extra.merge(other_extra) - assert extra.custom == "keep" + assert extra.custom["custom"] == "keep" def test_db_delete(self, tmp_session): """Remove the other extra from the db if it exists.""" diff --git a/tests/library/test_lib_item.py b/tests/library/test_lib_item.py index a4d36c99..005c6821 100644 --- a/tests/library/test_lib_item.py +++ b/tests/library/test_lib_item.py @@ -1,8 +1,5 @@ """Test shared library functionality.""" - -import pytest - import moe from moe.config import ExtraPlugin, MoeSession from moe.library import Album, Extra, Track @@ -12,58 +9,40 @@ class LibItemPlugin: """Plugin that implements the library item hooks for testing.""" - @staticmethod - @moe.hookimpl - def create_custom_album_fields(): - """Adds relevant musicbrainz fields to a track.""" - return {"changed": None, "new": None, "removed": None} - - @staticmethod - @moe.hookimpl - def create_custom_extra_fields(): - """Adds relevant musicbrainz fields to a track.""" - return {"changed": None, "new": None, "removed": None} - - @staticmethod - @moe.hookimpl - def create_custom_track_fields(): - """Adds relevant musicbrainz fields to a track.""" - return {"changed": None, "new": None, "removed": None} - @staticmethod @moe.hookimpl def edit_changed_items(items): """Edit changed items.""" for item in items: - item.changed = "edited" + item.custom["changed"] = "edited" @staticmethod @moe.hookimpl def edit_new_items(items): """Edit new items.""" for item in items: - item.new = "edited" + item.custom["new"] = "edited" @staticmethod @moe.hookimpl def process_changed_items(items): """Process changed items.""" for item in items: - item.changed = "processed" + item.custom["changed"] = "processed" @staticmethod @moe.hookimpl def process_new_items(items): """Process new items.""" for item in items: - item.new = "processed" + item.custom["new"] = "processed" @staticmethod @moe.hookimpl def process_removed_items(items): """Process removed items.""" for item in items: - item.removed = "processed" + item.custom["removed"] = "processed" class TestHooks: @@ -79,9 +58,6 @@ def test_edit_changed_items(self, tmp_config): album = album_factory() extra = extra_factory() track = track_factory() - assert not album.changed - assert not extra.changed - assert not track.changed session = MoeSession() session.add(album) @@ -89,14 +65,14 @@ def test_edit_changed_items(self, tmp_config): session.add(track) session.commit() - album.changed = "triggered" - extra.changed = "triggered" - track.changed = "triggered" + album.custom["changed"] = "triggered" + extra.custom["changed"] = "triggered" + track.custom["changed"] = "triggered" session.commit() - assert album.changed == "edited" - assert extra.changed == "edited" - assert track.changed == "edited" + assert album.custom["changed"] == "edited" + assert extra.custom["changed"] == "edited" + assert track.custom["changed"] == "edited" def test_edit_new_items(self, tmp_config): """Ensure plugins can implement the `edit_new_items` hook.""" @@ -108,9 +84,6 @@ def test_edit_new_items(self, tmp_config): album = album_factory() extra = extra_factory() track = track_factory() - assert not album.new - assert not extra.new - assert not track.new session = MoeSession() session.add(album) @@ -118,9 +91,9 @@ def test_edit_new_items(self, tmp_config): session.add(track) session.commit() - assert album.new == "edited" - assert extra.new == "edited" - assert track.new == "edited" + assert album.custom["new"] == "edited" + assert extra.custom["new"] == "edited" + assert track.custom["new"] == "edited" def test_process_changed_items(self, tmp_config): """Ensure plugins can implement the `edit_edited_items` hook.""" @@ -132,9 +105,6 @@ def test_process_changed_items(self, tmp_config): album = album_factory() extra = extra_factory() track = track_factory() - assert not album.changed - assert not extra.changed - assert not track.changed session = MoeSession() session.add(album) @@ -142,19 +112,19 @@ def test_process_changed_items(self, tmp_config): session.add(track) session.commit() - album.changed = "triggered" - extra.changed = "triggered" - track.changed = "triggered" + album.custom["changed"] = "triggered" + extra.custom["changed"] = "triggered" + track.custom["changed"] = "triggered" session.flush() - assert album.changed == "processed" - assert extra.changed == "processed" - assert track.changed == "processed" + assert album.custom["changed"] == "processed" + assert extra.custom["changed"] == "processed" + assert track.custom["changed"] == "processed" # changes won't persist session.commit() - assert album.changed != "processed" - assert extra.changed != "processed" - assert track.changed != "processed" + assert album.custom["changed"] != "processed" + assert extra.custom["changed"] != "processed" + assert track.custom["changed"] != "processed" def test_process_new_items(self, tmp_config): """Ensure plugins can implement the `edit_new_items` hook.""" @@ -166,9 +136,6 @@ def test_process_new_items(self, tmp_config): album = album_factory() extra = extra_factory() track = track_factory() - assert not album.new - assert not extra.new - assert not track.new session = MoeSession() session.add(album) @@ -176,15 +143,15 @@ def test_process_new_items(self, tmp_config): session.add(track) session.flush() - assert album.new == "processed" - assert extra.new == "processed" - assert track.new == "processed" + assert album.custom["new"] == "processed" + assert extra.custom["new"] == "processed" + assert track.custom["new"] == "processed" # changes won't persist session.commit() - assert album.new != "processed" - assert extra.new != "processed" - assert track.new != "processed" + assert album.custom["new"] != "processed" + assert extra.custom["new"] != "processed" + assert track.custom["new"] != "processed" def test_process_removed_items(self, tmp_config): """Ensure plugins can implement the `edit_removed_items` hook.""" @@ -196,9 +163,6 @@ def test_process_removed_items(self, tmp_config): album = album_factory() extra = extra_factory(album=album) track = track_factory(album=album) - assert not album.removed - assert not extra.removed - assert not track.removed session = MoeSession() session.add(album) @@ -211,9 +175,9 @@ def test_process_removed_items(self, tmp_config): session.delete(track) session.flush() - assert album.removed == "processed" - assert extra.removed == "processed" - assert track.removed == "processed" + assert album.custom["removed"] == "processed" + assert extra.custom["removed"] == "processed" + assert track.custom["removed"] == "processed" assert not session.query(Album).all() assert not session.query(Extra).all() assert not session.query(Track).all() @@ -224,59 +188,33 @@ class TestCustomFields: def test_db_persistence(self, tmp_session): """Ensure custom fields persist in the database.""" - track = track_factory( - custom_fields={"db": "persists", "my_list": [1, "change me"]} - ) + track = track_factory(db="persists", my_list=[1, "change me"]) - track.db = "persisted" - track.my_list[1] = "changed" + track.custom["db"] = "persisted" + track.custom["my_list"][1] = "changed" tmp_session.add(track) tmp_session.flush() db_track = tmp_session.query(Track).one() - assert db_track.db == "persisted" - assert db_track.my_list == [1, "changed"] + assert db_track.custom["db"] == "persisted" + assert db_track.custom["my_list"] == [1, "changed"] def test_db_changes(self, tmp_config): """Ensure custom fields changes also persist in the database.""" tmp_config(settings="default_plugins = []", tmp_db=True) track = track_factory( - custom_fields={ - "db": "persists", - "my_list": ["wow", "change me"], - "growing_list": ["one"], - } + db="persists", my_list=["wow", "change me"], growing_list=["one"] ) session = MoeSession() session.add(track) session.commit() - track.db = "persisted" - track.my_list[1] = "changed" - track.growing_list.append("two") + track.custom["db"] = "persisted" + track.custom["my_list"][1] = "changed" + track.custom["growing_list"].append("two") assert track in session.dirty db_track = session.query(Track).one() - assert db_track.db == "persisted" - assert db_track.my_list == ["wow", "changed"] - assert db_track.growing_list == ["one", "two"] - - def test_set_non_key(self): - """Don't set just any attribute as a custom field if the key doesn't exist.""" - track = track_factory(custom_key=1) - - with pytest.raises(KeyError): - assert track._custom_fields["custom_key"] == 1 - - def test_get_custom_field(self): - """We can get a custom field like a normal attribute.""" - track = track_factory(custom_fields={"custom": "field"}) - - assert track.custom == "field" - - def test_set_custom_field(self): - """We can set a custom field like a normal attribute.""" - track = track_factory(custom_fields={"custom_key": None}) - track.custom_key = "test" - - assert track._custom_fields["custom_key"] == "test" + assert db_track.custom["db"] == "persisted" + assert db_track.custom["my_list"] == ["wow", "changed"] + assert db_track.custom["growing_list"] == ["one", "two"] diff --git a/tests/library/test_track.py b/tests/library/test_track.py index c58f268b..d0e7ed00 100644 --- a/tests/library/test_track.py +++ b/tests/library/test_track.py @@ -13,12 +13,6 @@ class MyTrackPlugin: """Plugin that implements the extra hooks for testing.""" - @staticmethod - @moe.hookimpl - def create_custom_track_fields(): - """Create a new custom field.""" - return {"no_default": None, "default": "value"} - @staticmethod @moe.hookimpl def is_unique_track(track, other): @@ -37,14 +31,6 @@ def read_custom_tags(track_path, album_fields, track_fields): class TestHooks: """Test track hooks.""" - def test_create_custom_fields(self, tmp_config): - """Plugins can define new custom fields.""" - tmp_config(extra_plugins=[ExtraPlugin(MyTrackPlugin, "track_plugin")]) - track = track_factory() - - assert not track.no_default - assert track.default == "value" - def test_is_unique_track(self, tmp_config): """Plugins can add additional unique constraints.""" tmp_config(extra_plugins=[ExtraPlugin(MyTrackPlugin, "track_plugin")]) @@ -105,6 +91,20 @@ def test_use_given_artist(self): assert track.artist == "use me" + def test_custom_fields(self): + """Custom fields can be assigned using kwargs.""" + track = track_factory() + + new_track = Track( + track.album, + track.path, + track.title, + track.track_num, + custom="custom", + ) + + assert new_track.custom["custom"] == "custom" + class TestMetaInit: """Test MetaTrack initialization.""" @@ -324,6 +324,15 @@ def test_overwrite(self): assert track.title == "2" + def test_custom_fields(self): + """Merge custom fields too.""" + track = track_factory(custom="") + other_track = track_factory(custom="new") + + track.merge(other_track) + + assert track.custom["custom"] == "new" + class TestProperties: """Test various track properties.""" @@ -369,8 +378,8 @@ class TestListDuplicates: def test_genre(self, tmp_session): """Duplicate genres don't error.""" - track1 = track_factory(genre="pop") - track2 = track_factory(genre="pop") + track1 = track_factory(genres={"pop"}) + track2 = track_factory(genres={"pop"}) tmp_session.add(track1) tmp_session.add(track2) diff --git a/tests/plugins/add/test_add_core.py b/tests/plugins/add/test_add_core.py index 1f4f2c05..0f797d9d 100644 --- a/tests/plugins/add/test_add_core.py +++ b/tests/plugins/add/test_add_core.py @@ -85,8 +85,8 @@ def test_duplicate_list_fields_album(self, tmp_session): @pytest.mark.usefixtures("_tmp_add_config") def test_duplicate_list_field_tracks(self, tmp_session): """Duplicate list fields don't error when adding multiple tracks.""" - track1 = track_factory(genre="pop") - track2 = track_factory(genre="pop") + track1 = track_factory(genres={"pop"}) + track2 = track_factory(genres={"pop"}) add.add_item(track1) add.add_item(track2) diff --git a/tests/plugins/duplicate/test_dup_cli.py b/tests/plugins/duplicate/test_dup_cli.py index 1ed5a55c..57d1f5ff 100644 --- a/tests/plugins/duplicate/test_dup_cli.py +++ b/tests/plugins/duplicate/test_dup_cli.py @@ -109,8 +109,7 @@ def test_full_diff_album(self): num_discs=2, ) old_album.tracks[0].title = "really really long old title" - old_album.tracks[0].custom_fields.add("new field") - old_album.tracks[0].new_field = "whoa a new field" + old_album.tracks[0].custom["custom_field"] = "custom field!" assert old_album is not new_album diff --git a/tests/plugins/edit/test_edit_core.py b/tests/plugins/edit/test_edit_core.py index 977310af..bc69041c 100644 --- a/tests/plugins/edit/test_edit_core.py +++ b/tests/plugins/edit/test_edit_core.py @@ -75,10 +75,10 @@ def test_invalid_album_field(self): def test_custom_field(self): """We can edit custom fields.""" - track = track_factory(custom_fields={"my_title": "test"}) + track = track_factory(my_title="test") edit.edit_item(track, "my_title", "new") - assert track.my_title == "new" + assert track.custom["my_title"] == "new" class TestPluginRegistration: diff --git a/tests/plugins/musicbrainz/test_mb_cli.py b/tests/plugins/musicbrainz/test_mb_cli.py index 71444389..a3538e36 100644 --- a/tests/plugins/musicbrainz/test_mb_cli.py +++ b/tests/plugins/musicbrainz/test_mb_cli.py @@ -39,7 +39,7 @@ def test_track(self, mock_query): """Tracks associated album's are used.""" cli_args = ["mbcol", "*"] track = track_factory() - track.album.mb_album_id = "123" + track.album.custom["mb_album_id"] = "123" mock_query.return_value = [track] with patch( @@ -54,7 +54,7 @@ def test_extra(self, mock_query): """Extras associated album's are used.""" cli_args = ["mbcol", "-e", "*"] extra = extra_factory() - extra.album.mb_album_id = "123" + extra.album.custom["mb_album_id"] = "123" mock_query.return_value = [extra] with patch( @@ -68,7 +68,7 @@ def test_extra(self, mock_query): def test_album(self, mock_query): """Albums associated releases are used.""" cli_args = ["mbcol", "-a", "*"] - album = album_factory(custom_fields={"mb_album_id": "123"}) + album = album_factory(mb_album_id="123") mock_query.return_value = [album] with patch( @@ -83,7 +83,7 @@ def test_remove(self, mock_query): """Releases are removed from a collection if `--remove` option used.""" cli_args = ["mbcol", "--remove", "*"] track = track_factory() - track.album.mb_album_id = "123" + track.album.custom["mb_album_id"] = "123" mock_query.return_value = [track] with patch( @@ -98,7 +98,7 @@ def test_add(self, mock_query): """Releases are added to a collection if `--add` option used.""" cli_args = ["mbcol", "--add", "*"] track = track_factory() - track.album.mb_album_id = "123" + track.album.custom["mb_album_id"] = "123" mock_query.return_value = [track] with patch( diff --git a/tests/plugins/musicbrainz/test_mb_core.py b/tests/plugins/musicbrainz/test_mb_core.py index bcca6989..d9f7b0b0 100644 --- a/tests/plugins/musicbrainz/test_mb_core.py +++ b/tests/plugins/musicbrainz/test_mb_core.py @@ -153,7 +153,7 @@ def test_auto_remove_true(self, mb_config): ) as mock_rm_releases_call: mb_config.pm.hook.process_removed_items(items=[album]) - mock_rm_releases_call.assert_called_once_with({album.mb_album_id}) + mock_rm_releases_call.assert_called_once_with({album.custom["mb_album_id"]}) def test_all_items(self, mb_config): """Remove all items from the collection if `auto_remove` is true.""" @@ -167,7 +167,7 @@ def test_all_items(self, mb_config): mb_config.pm.hook.process_removed_items(items=[album1, album2]) mock_rm_releases_call.assert_called_once_with( - {album1.mb_album_id, album2.mb_album_id} + {album1.custom["mb_album_id"], album2.custom["mb_album_id"]} ) def test_auto_remove_false(self, mb_config): @@ -223,7 +223,7 @@ def test_auto_add_true(self, mb_config): ) as mock_add_releases_call: mb_config.pm.hook.process_new_items(items=[album]) - mock_add_releases_call.assert_called_once_with({album.mb_album_id}) + mock_add_releases_call.assert_called_once_with({album.custom["mb_album_id"]}) def test_auto_add_false(self, mb_config): """Don't add any items if `auto_add` set to false.""" @@ -278,12 +278,13 @@ def test_sync_album(self, mb_config): ) as mock_id: config.CONFIG.pm.hook.sync_metadata(item=old_album) - mock_id.assert_called_once_with(old_album.mb_album_id) + mock_id.assert_called_once_with(old_album.custom["mb_album_id"]) assert old_album.title == "synced" def test_sync_track(self, mb_config): """Tracks are synced with musicbrainz when called.""" old_track = track_factory(title="unsynced", mb_track_id="1") + old_track.album.custom["mb_album_id"] = "2" new_track = track_factory(title="synced") with patch.object( @@ -292,7 +293,7 @@ def test_sync_track(self, mb_config): config.CONFIG.pm.hook.sync_metadata(item=old_track) mock_id.assert_called_once_with( - old_track.mb_track_id, old_track.album.mb_album_id + old_track.custom["mb_track_id"], old_track.album.custom["mb_album_id"] ) assert old_track.title == "synced" @@ -453,8 +454,8 @@ def test_read_write(self, tmp_config): new_track = Track.from_file(track.path) - assert new_track.album.mb_album_id == "album id" - assert new_track.mb_track_id == "track id" + assert new_track.album.custom["mb_album_id"] == "album id" + assert new_track.custom["mb_track_id"] == "track id" class TestAddReleasesToCollection: diff --git a/tests/plugins/test_list.py b/tests/plugins/test_list.py index 6d82f2ea..2acaaf65 100644 --- a/tests/plugins/test_list.py +++ b/tests/plugins/test_list.py @@ -109,6 +109,16 @@ def test_info_track(self, mock_query): moe.cli.main(cli_args) + def test_custom_info(self, capsys, mock_query): + """Output custom fields as well.""" + cli_args = ["list", "--info", "*"] + mock_query.return_value = [track_factory(custom="show me")] + + moe.cli.main(cli_args) + + output = capsys.readouterr().out.split("\n") + assert "custom: show me" in output + class TestPluginRegistration: """Test the `plugin_registration` hook implementation.""" diff --git a/tests/test_query.py b/tests/test_query.py index 05b838d3..4b761986 100644 --- a/tests/test_query.py +++ b/tests/test_query.py @@ -238,35 +238,35 @@ def test_missing_extras_tracks(self, tmp_session): def test_custom_fields(self, tmp_session): """We can query a custom field.""" - album = album_factory(custom_fields={"custom": "album"}) - extra_factory(album=album, custom_fields={"custom": "extra"}) - track_factory(album=album, custom_fields={"custom": "track"}) + album = album_factory(blah="album") + extra_factory(album=album, blah="extra") + track_factory(album=album, blah="track") tmp_session.add(album) tmp_session.flush() - assert query("a:custom:album t:custom:track e:custom:extra", "album") + assert query("a:blah:album t:blah:track e:blah:extra", "album") def test_custom_field_regex(self, tmp_session): """We can regex query a custom field.""" - album = album_factory(custom_fields={"custom": "album"}) - extra_factory(album=album, custom_fields={"custom": 3}) - track_factory(album=album, custom_fields={"custom": "track"}) + album = album_factory(blah="album") + extra_factory(album=album, blah=3) + track_factory(album=album, blah="track") tmp_session.add(album) tmp_session.flush() - assert query("a:custom::albu. t:custom::trac. e:custom::3", "album") + assert query("a:blah::albu. t:blah::trac. e:blah::3", "album") def test_custom_list_field(self, tmp_session): """We can query custom list fields.""" - album = album_factory(custom_fields={"custom": ["album", 1]}) - extra_factory(album=album, custom_fields={"custom": ["extra", 2]}) - track_factory(album=album, custom_fields={"custom": ["track", 3]}) + album = album_factory(blah=["album", 1]) + extra_factory(album=album, blah=["extra", 2]) + track_factory(album=album, blah=["track", 3]) tmp_session.add(album) tmp_session.flush() - assert query("a:custom:album t:custom:track e:custom:extra", "album") - assert query("a:custom:1 e:custom:2 t:custom:3", "album") - assert query("t:custom:3 t:custom:track", "album") + assert query("a:blah:album t:blah:track e:blah:extra", "album") + assert query("a:blah:1 e:blah:2 t:blah:3", "album") + assert query("t:blah:3 t:blah:track", "album")