-
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
#2579 Adding styles to discogs plugin #3251
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.
Here are a few suggestions!
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.
Awesome; thank you for looking into this! Here are some suggestions.
@@ -4,7 +4,7 @@ | |||
# and then run "tox" from this directory. | |||
|
|||
[tox] | |||
envlist = py27-test, py37-test, py38-test, py27-flake8, docs | |||
envlist = py27-test, py37-test, py27-flake8, docs |
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.
Not sure why this is showing up in the diff? py38-test
is disabled by default on master.
beetsplug/discogs.py
Outdated
if style is None: | ||
self._log.debug('Style not Found') | ||
elif not style: | ||
return style |
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 seems to return []
, i.e., an empty list, instead of a string. I think it should be fine to remove this entire elif
case: if the string is empty, then 'something'.join(sorted([]))
will return ''
, which is probably what we want.
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.
:D Think there was some confusion. When i was checking len == 0 it should have been len== 1. If there is only one style tag in the response then I want to just return that style tag as is with no formatting. So I guess I just needed to chage len == 0 to len == 1. Hope this clarifies what I am trying to achieve. If this makes sense I will make the change.
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.
It does! But I still don't think this elif
case is necessary for that—a single style in the list will still "do nothing" when joined. For example:
>>> ', '.join(['foo'])
'foo'
…so the one-element list case is also handled by the general case.
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.
👍 fixed
Awesome; thank you!! Merged with a changelog entry. |
#2579 Adding styles to discogs plugin
Hi did this feature ever get added to the plugin? If so how do I use it? Thanks |
Thank you for this feature! One issue to report: It looks like the styles tag is only being written to the beets library, and not to the id3 tag itself.
|
Indeed; |
Adding styles to discogs plugin. Closes #2579.