-
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 ability for 'missing' plugin to show missing albums by library artists #2481
Conversation
@qlyoung, thanks for your PR! By analyzing the history of the files in this pull request, we identified @sampsyo, @tomjaspers and @geigerzaehler to be potential reviewers. |
beetsplug/missing.py
Outdated
def _normalize_caseless(text): | ||
"""unicode case normalization | ||
""" | ||
return unicodedata.normalize("NFKD", text.casefold()) |
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.
casefold()
requires python 3.3+
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, anything else?
24e1a38
to
7e76439
Compare
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 looks awesome! I've actually wanted something like this for a long time. It's like an in-beets version of Muspy. Thanks for getting it started!
I made a few suggestions inline. It also looks like Travis is reporting that some tests as failed. Can you take a look and perhaps update the tests?
beetsplug/missing.py
Outdated
for artist, albums in albums_by_artist.items(): | ||
if artist[1] is None or artist[1] == "": | ||
print_("[!] No musicbrainz ID for artist {}, skipping." | ||
.format(artist[0]), stream=sys.stderr) |
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 these prints to stderr, you can just use our logging system instead. Use self._log.info(...)
.
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.
perfect, will change
beetsplug/missing.py
Outdated
@@ -17,6 +18,12 @@ | |||
""" | |||
from __future__ import division, absolute_import, print_function | |||
|
|||
import time | |||
import beets | |||
import musicbrainzngs as m |
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.
As silly as it seems, can you please use the long "musicbrainzngs" name? This helps readability quite a bit.
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.
will do
beetsplug/missing.py
Outdated
albums_by_artist[artist] = artistalbs | ||
|
||
# build dict mapping artist to list of all albums | ||
m.set_useragent('beets', beets.__version__, 'http://beets.io/') |
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.
Why is this necessary?
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.
built it into a separate tool, forgot to remove for port into plugin; will remove
beetsplug/missing.py
Outdated
print_("{}{}".format(prefix_miss, release)) | ||
|
||
# respect musicbrainz rate limits | ||
time.sleep(1) |
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 library contains internal rate limiting, so we don't need extra waiting here.
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.
will remove
beetsplug/missing.py
Outdated
for release in releases: | ||
missing.append(release) | ||
for alb in albums: | ||
# compare by title, don't care about specific releases |
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.
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.
good idea, was looking for something better than title but not super familiar with mb api -- will implement
beetsplug/missing.py
Outdated
prefix_miss = "{} - ".format(artist[0]) | ||
|
||
for release in missing_titles: | ||
print_("{}{}".format(prefix_miss, release)) |
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 (and the previous stanza would be more readable if they just used:
for release in missing_titles:
print_("{} - {}".format(artist[0], release))
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.
agreed, had the ability to show both present and missing albums in the same output in a previous version, but this can be done with standard shell utils, so no need for it; will change
All requested changes made. Regarding tests, apparently they don't like |
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.
Nice; thanks! And thanks for adding documentation too. I just have a few low-level coding suggestions based on the revisions.
beets/ui/__init__.py
Outdated
else: | ||
# In our test harnesses (e.g., DummyOut), sys.stdout.buffer | ||
# does not exist. We instead just record the text string. | ||
sys.stdout.write(txt) | ||
stream.write(txt) |
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. Now that you're using the logging infrastructure, I believe these changes to print_
are unnecessary. Could you please back them out?
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.
Yep
beetsplug/missing.py
Outdated
@@ -106,35 +111,95 @@ def __init__(self): | |||
self._command.parser.add_option( | |||
u'-t', u'--total', dest='total', action='store_true', | |||
help=u'count total of missing tracks') | |||
self._command.parser.add_option( | |||
u'-a', u'--albums', dest='albums', action='store_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.
In other commands, such as beet ls
, a similar (but different) flag is always called --album
, not --albums
. Can we please stick with that name for consistency? (And the same for the config option name, of course.)
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.
Yep
beetsplug/missing.py
Outdated
if count: | ||
if _missing_count(album): | ||
print_(format(album, fmt)) | ||
def _missing_tracks(self, lib, args): |
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.
Instead of taking a parameter called args
, which is bytestrings, these two functions should probably take a query called query
—the result of decargs(args)
. This should make the method signatures a little clearer.
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.
Will do
beetsplug/missing.py
Outdated
print_(format(album, fmt)) | ||
def _missing_tracks(self, lib, args): | ||
"""Logic for determining missing tracks from each album in the library | ||
(the usual 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.
Suggested docstring that uses an imperative voice (the standard in beets):
Print a listing of tracks missing from each album in the library matching
query
.
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.
Will do
self._command.parser.add_format_option() | ||
|
||
def commands(self): | ||
def _miss(lib, opts, args): | ||
self.config.set_args(opts) | ||
count = self.config['count'].get() | ||
total = self.config['total'].get() |
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.
Should we document the fact that count
and total
are ignored in album mode?
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.
Noted for count
, implemented for total
beetsplug/missing.py
Outdated
""" | ||
albums = lib.albums(decargs(args)) | ||
# build dict mapping artist to list of their albums in library | ||
albums_by_artist = {} |
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 might be a good place for a defaultdict(list)
.
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.
Yep
beetsplug/missing.py
Outdated
for artist, albums in albums_by_artist.items(): | ||
if artist[1] is None or artist[1] == "": | ||
self._log.info( | ||
u"No musicbrainz ID for artist {}, skipping.", |
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 like it will print "no MBID for artist Foo" if "Foo" appears once with an MBID and once without. I suppose that's a reasonable sacrifice, but it seems a little strange.
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 about "no MBID for artist Foo in album {album}"? Seems weird too I guess.
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.
Sure; sounds good.
Passing -a to 'beet missing' shows albums missing by all artists in the library. Signed-off-by: Quentin Young <[email protected]>
* Use full name for musicbrainzngs import * Use beets internal logging facilities * Match releases by release id * Convert some strings to Unicode * Remove unnecessary MB rate-limiting * Remove unnecessary imports * Follow beets convention for `--album` option * Follow beets convention for imperative docstrings * Simplify method signatures * Use defaultdict(list) where appropriate * Clarify missing MBID log message Signed-off-by: Quentin Young <[email protected]>
Signed-off-by: Quentin Young <[email protected]>
Produces a count of missing albums without listing them Signed-off-by: Quentin Young <[email protected]>
All suggestions implemented, plus New message for missing MBID looks like
|
This looks perfect. Thank you again for implementing this! I'll run this on my own collection immediately. 😃 |
Add ability for 'missing' plugin to show missing albums by library artists
Passing the
-a
flag shows, for each artist in the library, the albums by that artist which are not in the library.Also includes a minor enhancement to allow
beets.ui.print_
to print tostderr
.Example output (truncated):