Remove debug commands, add mblookup command #249
Conversation
RecursiveForest
force-pushed
the
remove-debug
branch
from
d7a3684
to
6c2ffae
Compare
| import urllib2 | ||
|
|
||
| import whipper | ||
|
|
||
| import logging |
There was a problem hiding this comment.
I would probably do these lines like:
[…]
"""
import logging
import urllib2
import whipper
logger = …There was a problem hiding this comment.
It's standard to import logging on its own line right above where you define logger, so I'd like to keep it where it is.
There was a problem hiding this comment.
Right, but usually you first import system libs, then third party libs, then your own. I think that is what @Freso meant?
There was a problem hiding this comment.
It is! But I'm saying that despite pep8 it's common practice to import logging on its own line right above logger, out of standard order. As far as I can tell I'm otherwise following the standard import order. If someone feels really strongly about this I'm open to changing it, but this is the style whipper is currently written in and if we decide to change the import order for logging we should do it wholesale in another PR. (I'm fine discussing it here, however.)
whipper/command/mblookup.py
Outdated
| print ' Artist: %s' % md.artist.encode('utf-8') | ||
| print ' Title: %s' % md.title.encode('utf-8') | ||
| print ' Type: %s' % md.releaseType.encode('utf-8') # noqa: E501 | ||
| print ' URL: %s' % md.url |
There was a problem hiding this comment.
The other outputs has the %s indented to align with each other, so this one will look out of place when printed. (This seems to be a carry-over from the old code though, so should probably be fixed in its own commit (if not PR).)
There was a problem hiding this comment.
good catch-- should be fixed in this PR.
whipper/command/main.py
Outdated
| 'drive': drive.Drive, | ||
| 'offset': offset.Offset, | ||
| 'image': image.Image | ||
| 'image': image.Image, | ||
| 'mblookup': mblookup.MBLookup |
There was a problem hiding this comment.
This lines doesn't align with the rest of the dict. They should probably all have another space added, or all made to only have one space total. (Of course, if you change mblookup to lookup then it should be fine without altering all the other lines.)
There was a problem hiding this comment.
Yeah I know-- I should un-align the rest of them for consistency.
|
Changes look good - didn't test the code. ACK from me. |
|
mblookup for filename, mblookup for cmd seems sensible to me. |
| discId = unicode(self.options.mbdiscid) | ||
| except IndexError: | ||
| print 'Please specify a MusicBrainz disc id.' | ||
| return 3 |
There was a problem hiding this comment.
The return value of 3 has no specific meaning, right? (just returning non-zero)
There was a problem hiding this comment.
It may be semantic, but I haven't checked if it means anything by a particular standard. It's left over from morituri.
|
Will test it today: I'd like to merge this PR then tag a new release ( |
the
debugcommands were all variously useless internals-exposing tools that were not of significant debugging utility with the exception ofwhipper debug musicbrainzngs, which I have elevated to the top-level commandwhipper mblookup.