-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
Add bracket argument to aunique #2399
Changes from 4 commits
377a2a6
3a967df
d10df34
eaa2161
04f7915
672fc36
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1447,13 +1447,15 @@ def tmpl_time(s, fmt): | |
cur_fmt = beets.config['time_format'].as_str() | ||
return time.strftime(fmt, time.strptime(s, cur_fmt)) | ||
|
||
def tmpl_aunique(self, keys=None, disam=None): | ||
def tmpl_aunique(self, keys=None, disam=None, bracket=None): | ||
"""Generate a string that is guaranteed to be unique among all | ||
albums in the library who share the same set of keys. A fields | ||
from "disam" is used in the string if one is sufficient to | ||
disambiguate the albums. Otherwise, a fallback opaque value is | ||
used. Both "keys" and "disam" should be given as | ||
whitespace-separated lists of field names. | ||
whitespace-separated lists of field names, while "bracket" is a | ||
pair of characters to be used as brackets surrounding the | ||
disambiguator or a white space to have no brackets. | ||
""" | ||
# Fast paths: no album, no item or library, or memoized value. | ||
if not self.item or not self.lib: | ||
|
@@ -1467,8 +1469,18 @@ def tmpl_aunique(self, keys=None, disam=None): | |
|
||
keys = keys or 'albumartist album' | ||
disam = disam or 'albumtype year label catalognum albumdisambig' | ||
bracket = bracket or '[]' | ||
keys = keys.split() | ||
disam = disam.split() | ||
bracket = None if bracket == ' ' else list(bracket) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To avoid crashes, maybe this should check that |
||
|
||
# Assign a left and right bracket from bracket list. | ||
if bracket: | ||
bracket_l = bracket[0] | ||
bracket_r = bracket[1] | ||
else: | ||
bracket_l = u'' | ||
bracket_r = u'' | ||
|
||
album = self.lib.get_album(self.item) | ||
if not album: | ||
|
@@ -1502,13 +1514,19 @@ def tmpl_aunique(self, keys=None, disam=None): | |
|
||
else: | ||
# No disambiguator distinguished all fields. | ||
res = u' {0}'.format(album.id) | ||
res = u' {1}{0}{2}'.format(album.id, bracket_l, bracket_r) | ||
self.lib._memotable[memokey] = res | ||
return res | ||
|
||
# Flatten disambiguation value into a string. | ||
disam_value = album.formatted(True).get(disambiguator) | ||
res = u' [{0}]'.format(disam_value) | ||
|
||
# Return empty string if disambiguator is empty. | ||
if disam_value: | ||
res = u' {1}{0}{2}'.format(disam_value, bracket_l, bracket_r) | ||
else: | ||
res = u'' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm… is there a cleaner way we can check for an empty disambiguation string? Perhaps, if this appears earlier, we can just use |
||
|
||
self.lib._memotable[memokey] = res | ||
return res | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -71,8 +71,8 @@ These functions are built in to beets: | |
For example, "café" becomes "cafe". Uses the mapping provided by the | ||
`unidecode module`_. See the :ref:`asciify-paths` configuration | ||
option. | ||
* ``%aunique{identifiers,disambiguators}``: Provides a unique string to | ||
disambiguate similar albums in the database. See :ref:`aunique`, below. | ||
* ``%aunique{identifiers,disambiguators,brackets}``: Provides a unique string | ||
to disambiguate similar albums in the database. See :ref:`aunique`, below. | ||
* ``%time{date_time,format}``: Return the date and time in any format accepted | ||
by `strftime`_. For example, to get the year some music was added to your | ||
library, use ``%time{$added,%Y}``. | ||
|
@@ -112,21 +112,28 @@ will expand to "[2008]" for one album and "[2010]" for the other. The | |
function detects that you have two albums with the same artist and title but | ||
that they have different release years. | ||
|
||
For full flexibility, the ``%aunique`` function takes two arguments, each of | ||
which are whitespace-separated lists of album field names: a set of | ||
*identifiers* and a set of *disambiguators*. Any group of albums with identical | ||
values for all the identifiers will be considered "duplicates". Then, the | ||
function tries each disambiguator field, looking for one that distinguishes each | ||
of the duplicate albums from each other. The first such field is used as the | ||
result for ``%aunique``. If no field suffices, an arbitrary number is used to | ||
distinguish the two albums. | ||
For full flexibility, the ``%aunique`` function takes three arguments. The | ||
first two are whitespace-separated lists of album field names: a set of | ||
*identifiers* and a set of *disambiguators*. The third argument is a pair of | ||
characters used to surround the disambiguator. | ||
|
||
Any group of albums with identical values for all the identifiers will be | ||
considered "duplicates". Then, the function tries each disambiguator field, | ||
looking for one that distinguishes each of the duplicate albums from each | ||
other. The first such field is used as the result for ``%aunique``. If no field | ||
suffices, an arbitrary number is used to distinguish the two albums. | ||
|
||
The default identifiers are ``albumartist album`` and the default disambiguators | ||
are ``albumtype year label catalognum albumdisambig``. So you can get reasonable | ||
disambiguation behavior if you just use ``%aunique{}`` with no parameters in | ||
your path forms (as in the default path formats), but you can customize the | ||
disambiguation if, for example, you include the year by default in path formats. | ||
|
||
The default characters used as brackets are ``[]``. To change this, provide a | ||
third argument to the ``%aunique`` function consisting of two characters: the left | ||
and right brackets. Or, to turn off bracketing entirely, use a single blank | ||
space. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great; this sounds good! But just to triple-check: is it necessary for the "null option" here to be a single space? That seems a little counterintuitive, but if there's a technical reason we can't use the empty string instead it's fine. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, I set it up to default to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right—I meant that, to get no brackets, you would use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, I think I got it. I was trying to distinguish |
||
|
||
One caveat: When you import an album that is named identically to one already in | ||
your library, the *first* album—the one already in your library— will not | ||
consider itself a duplicate at import time. This means that ``%aunique{}`` will | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -713,8 +713,8 @@ def test_use_fallback_numbers_when_identical(self): | |
album2.year = 2001 | ||
album2.store() | ||
|
||
self._assert_dest(b'/base/foo 1/the title', self.i1) | ||
self._assert_dest(b'/base/foo 2/the title', self.i2) | ||
self._assert_dest(b'/base/foo [1]/the title', self.i1) | ||
self._assert_dest(b'/base/foo [2]/the title', self.i2) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That looks like the right change to me! |
||
|
||
def test_unique_falls_back_to_second_distinguishing_field(self): | ||
self._setf(u'foo%aunique{albumartist album,month year}/$title') | ||
|
@@ -730,6 +730,24 @@ def test_unique_sanitized(self): | |
self._setf(u'foo%aunique{albumartist album,albumtype}/$title') | ||
self._assert_dest(b'/base/foo [foo_bar]/the title', self.i1) | ||
|
||
def test_drop_empty_disambig_string(self): | ||
album1 = self.lib.get_album(self.i1) | ||
album1.albumdisambig = None | ||
album2 = self.lib.get_album(self.i2) | ||
album2.albumdisambig = u'foo' | ||
album1.store() | ||
album2.store() | ||
self._setf(u'foo%aunique{albumartist album,albumdisambig}/$title') | ||
self._assert_dest(b'/base/foo/the title', self.i1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How do I add an empty field in the tests? My efforts yielded There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm; I suppose I assumed you had run into this with some music in your library. Maybe you can reuse the data from there? You could, for example, try setting some arbitrary field There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that is what I ran into with my library. I think adding the second album fixed it. |
||
|
||
def test_change_brackets(self): | ||
self._setf(u'foo%aunique{albumartist album,year,()}/$title') | ||
self._assert_dest(b'/base/foo (2001)/the title', self.i1) | ||
|
||
def test_remove_brackets(self): | ||
self._setf(u'foo%aunique{albumartist album,year, }/$title') | ||
self._assert_dest(b'/base/foo 2001/the title', self.i1) | ||
|
||
|
||
class PluginDestinationTest(_common.TestCase): | ||
def setUp(self): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a description of
bracket
to the docstring?