Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Formatted modify and import --set-field. #4095

Merged
merged 10 commits into from
Aug 21, 2022
Merged

Formatted modify and import --set-field. #4095

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
636e36e
allow templates/formatting when setting fields with modify
Duncaen Oct 6, 2021
819ba73
allow templates/formatting of set_fields on import
Duncaen Oct 6, 2021
5824d46
changelog: rewrite permissions cover art change
Duncaen Oct 6, 2021
a2030d1
changelog: import/modify field formatting
Duncaen Oct 6, 2021
795bc2e
compile modify templates only once
Duncaen Dec 15, 2021
0456c8f
test multiple items in test_modify_formatted
Duncaen Dec 15, 2021
7af40db
Merge branch 'master' into formatted-modify
sampsyo Aug 20, 2022
dad918e
Out-of-date changelog fixes
sampsyo Aug 20, 2022
b207224
Further document formatted modify with examples
sampsyo Aug 20, 2022
fb9e95b
Fix some long lines
sampsyo Aug 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions beets/importer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,9 +584,9 @@ def set_fields(self, lib):
displayable_path(self.paths),
field,
value)
self.album[field] = value
self.album.set_parse(field, format(self.album, value))
Copy link
Member

Choose a reason for hiding this comment

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

Looking a little closer, I couldn't quite grok what the effect of using set_parse instead of just ordinary item assignment would be. Just out of curiosity, did you run into any situations where this behaved different?

I notice that beet modify uses print_and_modify, which in turn uses obj.update, which I assume will eventually end up using ordinary attribute assignment. Is there any chance that beet modify will now behave differently from setting attributes on import?

Copy link
Member

Choose a reason for hiding this comment

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

Oops; I now see that we were explicitly using _parse to get this effect in beet modify, so this is actually now more consistent than it was before. Yay!

for item in items:
item[field] = value
item.set_parse(field, format(item, value))
with lib.transaction():
for item in items:
item.store()
Expand Down Expand Up @@ -963,7 +963,7 @@ def set_fields(self, lib):
displayable_path(self.paths),
field,
value)
self.item[field] = value
self.item.set_parse(field, format(self.item, value))
self.item.store()


Expand Down
7 changes: 3 additions & 4 deletions beets/ui/commands.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1411,9 +1411,6 @@ def modify_items(lib, mods, dels, query, write, move, album, confirm):
# Parse key=value specifications into a dictionary.
model_cls = library.Album if album else library.Item

for key, value in mods.items():
mods[key] = model_cls._parse(key, value)

# Get the items to modify.
items, albums = _do_query(lib, query, album, False)
objs = albums if album else items
Expand All @@ -1424,7 +1421,9 @@ def modify_items(lib, mods, dels, query, write, move, album, confirm):
.format(len(objs), 'album' if album else 'item'))
changed = []
for obj in objs:
if print_and_modify(obj, mods, dels) and obj not in changed:
obj_mods = {key: model_cls._parse(key, format(obj, value))
for key, value in mods.items()}
Copy link
Member

Choose a reason for hiding this comment

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

It occurs to me that we are losing a couple of optimizations by moving this logic inside the loop:

  1. The template is compiled n times. This is not required; we could hoist the template compilation out of the loop and reuse it in place of the value string.
  2. _parse is called n times instead of once. This is kind of fundamental; the formatted value might be different and need to be parsed differently on every iteration. We could consider detecting whether the value actually uses any templating and avoid this if the value happens to be a constant—the way beet modify used to behave.

I propose we attempt to address 1 now, as part of this PR, and create a separate issue for addressing item 2. The former should (hopefully) be easy. The latter will require a bit more complexity and IMO is not really necessary to ship this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like @Duncaen added a couple new commits to this PR, is this review still accurate?

Copy link
Member

Choose a reason for hiding this comment

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

No; looks like this is now rearranged a bit! I think this is due for another look—it may be ready to go as-is?

if print_and_modify(obj, obj_mods, dels) and obj not in changed:
changed.append(obj)

# Still something to do?
Expand Down
7 changes: 6 additions & 1 deletion docs/changelog.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,14 @@ Major new features:
* :doc:`/plugins/albumtypes`: An accompanying plugin for formatting
``albumtypes``. Thanks to :user:`edgars-supe`.

* The :ref:`modify-cmd` and :ref:`import-cmd` can now use
:doc:`/reference/pathformat` formats when setting fields.
:bug:`488`

Other new things:

* Permissions plugin now sets cover art permissions to the file permissions.
* :doc:`/plugins/permissions`: Set cover art permissions to configured file
permission.
* :doc:`/plugins/unimported`: Support excluding specific
subdirectories in library.

Expand Down
5 changes: 5 additions & 0 deletions docs/reference/cli.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,8 @@ Optional command flags:
Multiple IDs can be specified by simply repeating the option several times.

* You can supply ``--set field=value`` to assign `field` to `value` on import.
Values support the same template syntax as beets'
:doc:`path formats <pathformat>`.
These assignments will merge with (and possibly override) the
:ref:`set_fields` configuration dictionary. You can use the option multiple
times on the command line, like so::
Expand Down Expand Up @@ -263,6 +265,9 @@ artist="Tom Tom Club"`` will change the artist for the track "Genius of Love."
To remove fields (which is only possible for flexible attributes), follow a
field name with an exclamation point: ``field!``.

Values support the same template syntax as beets'
:doc:`path formats <pathformat>`.

The ``-a`` switch operates on albums instead of individual tracks. Without
this flag, the command will only change *track-level* data, even if all the
tracks belong to the same album. If you want to change an *album-level* field,
Expand Down
3 changes: 3 additions & 0 deletions docs/reference/config.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -706,6 +706,9 @@ Here's an example::
Other field/value pairs supplied via the ``--set`` option on the command-line
override any settings here for fields with the same name.

Values support the same template syntax as beets'
:doc:`path formats <pathformat>`.

Fields are set on both the album and each individual track of the album.
Fields are persisted to the media files of each track.

Expand Down
14 changes: 12 additions & 2 deletions test/test_importer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,7 +554,8 @@ def test_set_fields(self):

config['import']['set_fields'] = {
'collection': collection,
'genre': genre
'genre': genre,
'title': "$title - formatted",
}

# As-is item import.
Expand All @@ -566,6 +567,7 @@ def test_set_fields(self):
item.load() # TODO: Not sure this is necessary.
self.assertEqual(item.genre, genre)
self.assertEqual(item.collection, collection)
self.assertEqual(item.title, "Tag Title 1 - formatted")
# Remove item from library to test again with APPLY choice.
item.remove()

Expand All @@ -579,6 +581,7 @@ def test_set_fields(self):
item.load()
self.assertEqual(item.genre, genre)
self.assertEqual(item.collection, collection)
self.assertEqual(item.title, "Applied Title 1 - formatted")


class ImportTest(_common.TestCase, ImportHelper):
Expand Down Expand Up @@ -743,7 +746,8 @@ def test_set_fields(self):
config['import']['set_fields'] = {
'genre': genre,
'collection': collection,
'comments': comments
'comments': comments,
'album': "$album - formatted",
}

# As-is album import.
Expand All @@ -765,6 +769,9 @@ def test_set_fields(self):
self.assertEqual(
item.get("comments", with_album=False),
comments)
self.assertEqual(
item.get("album", with_album=False),
"Tag Album - formatted")
# Remove album from library to test again with APPLY choice.
album.remove()

Expand All @@ -788,6 +795,9 @@ def test_set_fields(self):
self.assertEqual(
item.get("comments", with_album=False),
comments)
self.assertEqual(
item.get("album", with_album=False),
"Applied Album - formatted")


class ImportTracksTest(_common.TestCase, ImportHelper):
Expand Down
14 changes: 14 additions & 0 deletions test/test_ui.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,13 @@ def test_selective_modify(self):
self.assertEqual(len(list(original_items)), 3)
self.assertEqual(len(list(new_items)), 7)

def test_modify_formatted(self):
item = self.lib.items().get()
orig_title = item.title
self.modify("title=${title} - append")
item.load()
self.assertEqual(item.title, f"{orig_title} - append")

# Album Tests

def test_modify_album(self):
Expand Down Expand Up @@ -318,6 +325,13 @@ def test_album_not_move(self):
item.read()
self.assertNotIn(b'newAlbum', item.path)

def test_modify_album_formatted(self):
item = self.lib.items().get()
orig_album = item.album
self.modify("--album", "album=${album} - append")
item.load()
self.assertEqual(item.album, f"{orig_album} - append")

# Misc

def test_write_initial_key_tag(self):
Expand Down