-
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
Conversation
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.
Aha! Thanks for diagnosing this—you're right that empty brackets look weird and we should dispense with them. And in addition, the new brackets parameter seems useful. I'm all for this.
It would be awesome to have small tests for the new functionality (dropping empty disambiguation strings and changing the brackets) and a changelog entry.
@@ -1447,7 +1447,7 @@ 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): |
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?
keys = keys.split() | ||
disam = disam.split() | ||
bracket = None if bracket is ' ' else list(bracket) |
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.
- Is there a reason the condition is
bracket is ' '
instead of using the empty string''
? is
might work sometimes for comparing strings, but==
is the right operator to use here.
>>> 'a' is 'a'
True
>>> 'a' * 100 is 'a' * 100
False
>>> 'a' * 100 == 'a' * 100
True
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.
The bracket == ' '
is to check for a white space in the third argument for %aunique
in order to remove any bracketing.
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.
Right—I guess the question is whether the empty string might be a better "use nothing" indicator than a space.
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) | ||
res = u' {1}{0}{2}'.format(disam_value, bracket[0] if bracket else u'', | ||
bracket[1] if bracket else u'') |
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.
Maybe this and the prior line with bracket-formatting would be more legible if the two brackets were assigned into variables?
if bracket:
bracket_l = bracket[0]
bracket_r = bracket[1]
else:
bracket_l = u''
bracket_r = u''
@@ -1502,13 +1504,20 @@ 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[0] if bracket else | |||
u'', bracket[1] if bracket else u'') |
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.
This is fine—I don't have a strong opinion about whether the brackets should appear here, but you're certainly right that it will look more consistent.
|
||
# Remove space and/or brackets if disambiguation value is empty | ||
if bracket and res == u' ' + ''.join(bracket) or res.isspace(): | ||
res = u'' |
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.
Hmm… is there a cleaner way we can check for an empty disambiguation string? Perhaps, if this appears earlier, we can just use if disam_value:
for the check.
|
||
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 ``[]``. If a single blank space is | ||
used, then the disambiguator will not be surrounded by anything. |
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.
For full clarity, maybe something like this:
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
That looks like the right change to me!
1fb3068
to
3a967df
Compare
album1 = self.lib.get_album(self.i1) | ||
album1.year = None | ||
album1.store() | ||
self._assert_dest(b'/base/foo/the title', self.i1) |
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.
How do I add an empty field in the tests? My efforts yielded [0000]
for year = None
and [1]
for everything else.
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.
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 foo
to be empty on one album and to have a known value on another, and then set up the %aunique
invocation to use that as the only (or first) disambiguator.
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.
Yeah, that is what I ran into with my library. I think adding the second album fixed it.
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.
Looking great. Here's one last suggestion and a question.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid crashes, maybe this should check that bracket
has length 2.
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I set it up to default to []
. I could change it to default to nothing, and that way we can get rid of the whole single space thing, or change from a single space to something else as the argument for no brackets. Is that what you mean?
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.
Right—I meant that, to get no brackets, you would use %aunique{foo,bar,}
instead of %aunique{foo,bar, }
. To use the default square brackets, you would leave the comma off: %aunique{foo,bar}
. In other words, the third argument to the function would be ''
for no brackets and None
for the default.
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.
OK, I think I got it. I was trying to distinguish ''
from None
with ' '
, but I guess that's not necessary.
Add bracket argument to aunique
Indexing a string works just fine.
Add bracket argument to aunique
Indexing a string works just fine.
Fantastic; thank you for finishing this up! It's all merged. ✨ |
A little background as to why I felt this was needed:
If I have two albums that are disambiguated by
albumdisambig
, and one album has that field empty, then it would have an empty[]
. In this case, I would only want the brackets gone.Also, if I do something like
[$year%aunique{}]
, then I would have two sets of brackets, with the inner set possibly empty. Plus, in this case, if the inner brackets are removed, then there is still a white space after the year and before the closing bracket.So this change does a few things:
album_id
is used as the disambiguator (I decided to add this for consistency, but maybe it was left out originally for a reason I'm not aware of).This will close #2397.
Finally, I think adding the brackets to the
album_id
fallback messed up the test forlibrary.py
. I fixed it, but I don't know if I did it correctly.And I'm totally fine if this request is considered unnecessary for whatever reason, especially if some of my changes look "ugly".