Merge pull request #357 from whipper-team/bugfix/idea-issues

Address warnings/errors from various static analysis tools

misc/offsets.py

@@ -64,4 +64,4 @@
     line = line[:-2] + '"'
     lines.append(line)
 
-print "\n".join(lines)
+print("\n".join(lines))

whipper/command/accurip.py

@@ -59,9 +59,7 @@ def do(self):
                 assert len(r.checksums) == r.num_tracks
                 assert len(r.confidences) == r.num_tracks
 
-                entry = {}
-                entry["confidence"] = r.confidences[track]
-                entry["response"] = i + 1
+                entry = {"confidence": r.confidences[track], "response": i + 1}
                 checksum = r.checksums[track]
                 if checksum in checksums:
                     checksums[checksum].append(entry)

whipper/command/basecommand.py

@@ -25,7 +25,7 @@
 # options) to the child command.
 
 
-class BaseCommand():
+class BaseCommand:
     """
     Register and handle whipper command arguments with ArgumentParser.
 

whipper/command/cd.py

@@ -66,6 +66,7 @@
 class _CD(BaseCommand):
     eject = True
 
+    # XXX: Pylint, parameters differ from overridden 'add_arguments' method
     @staticmethod
     def add_arguments(parser):
         parser.add_argument('-R', '--release-id',
@@ -156,10 +157,6 @@ def do(self):
             "full table's mb id %s differs from toc id mb %s" % (
             self.itable.getMusicBrainzDiscId(),
             self.ittoc.getMusicBrainzDiscId())
-        assert self.itable.accuraterip_path() == \
-            self.ittoc.accuraterip_path(), \
-            "full table's AR URL %s differs from toc AR URL %s" % (
-            self.itable.accuraterip_url(), self.ittoc.accuraterip_url())
 
         if self.program.metadata:
             self.program.metadata.discid = self.ittoc.getMusicBrainzDiscId()
@@ -191,6 +188,8 @@ def do(self):
         if self.options.eject in ('success', 'always'):
             utils.eject_device(self.device)
 
+        return None
+
     def doCommand(self):
         pass
 
@@ -203,6 +202,7 @@ class Info(_CD):
 
     # Requires opts.device
 
+    # XXX: Pylint, parameters differ from overridden 'add_arguments' method
     def add_arguments(self):
         _CD.add_arguments(self.parser)
 
@@ -226,6 +226,7 @@ class Rip(_CD):
     # Requires opts.record
     # Requires opts.device
 
+    # XXX: Pylint, parameters differ from overridden 'add_arguments' method
     def add_arguments(self):
         loggers = list(result.getLoggers())
         default_offset = None
@@ -244,7 +245,6 @@ def add_arguments(self):
                                  default='whipper',
                                  help=("logger to use (choose from: '%s" %
                                        "', '".join(loggers) + "')"))
-        # FIXME: get from config
         self.parser.add_argument('-o', '--offset',
                                  action="store", dest="offset",
                                  default=default_offset,
@@ -413,6 +413,7 @@ def _ripIfNotRipped(number):
                                                   len(self.itable.tracks),
                                                   extra))
                         break
+                    # FIXME: catching too general exception (Exception)
                     except Exception as e:
                         logger.debug('got exception %r on try %d', e, tries)
 

whipper/command/main.py

@@ -45,14 +45,15 @@ def main():
         logger.critical("SystemError: %s", e)
         if (isinstance(e, common.EjectError) and
                 cmd.options.eject in ('failure', 'always')):
+            # XXX: Pylint, instance of 'SystemError' has no 'device' member
             eject_device(e.device)
         return 255
     except RuntimeError as e:
         print(e)
         return 1
     except KeyboardInterrupt:
         return 2
-    except ImportError as e:
+    except ImportError:
         raise
     except task.TaskException as e:
         if isinstance(e.exception, ImportError):
@@ -74,11 +75,11 @@ def main():
 
 
 class Whipper(BaseCommand):
-    description = """whipper is a CD ripping utility focusing on accuracy over speed.
-
-whipper gives you a tree of subcommands to work with.
-You can get help on subcommands by using the -h option to the subcommand.
-"""
+    description = (
+        "whipper is a CD ripping utility focusing on accuracy over speed.\n\n"
+        "whipper gives you a tree of subcommands to work with.\n"
+        "You can get help on subcommands by using the -h option "
+        "to the subcommand.\n")
     no_add_help = True
     subcommands = {
         'accurip': accurip.AccuRip,

whipper/command/mblookup.py

@@ -42,3 +42,5 @@ def do(self):
                         j + 1, track.artist.encode('utf-8'),
                         track.title.encode('utf-8')
                     ))
+
+        return None

whipper/command/offset.py

@@ -88,13 +88,12 @@ def do(self):
         table = t.table
 
         logger.debug("CDDB disc id: %r", table.getCDDBDiscId())
-        responses = None
         try:
             responses = accurip.get_db_entry(table.accuraterip_path())
         except accurip.EntryNotFound:
             logger.warning("AccurateRip entry not found: drive offset "
                            "can't be determined, try again with another disc")
-            return
+            return None
 
         if responses:
             logger.debug('%d AccurateRip responses found.', len(responses))
@@ -133,7 +132,7 @@ def match(archecksums, track, responses):
                 logger.warning('cannot rip with offset %d...', offset)
                 continue
 
-            logger.debug('AR checksums calculated: %s %s', archecksums)
+            logger.debug('AR checksums calculated: %s', archecksums)
 
             c, i = match(archecksums, 1, responses)
             if c:
@@ -170,6 +169,8 @@ def match(archecksums, track, responses):
         logger.error('no matching offset found. '
                      'Consider trying again with a different disc')
 
+        return None
+
     def _arcs(self, runner, table, track, offset):
         # rips the track with the given offset, return the arcs checksums
         logger.debug('ripping track %r with offset %d...', track, offset)
@@ -196,9 +197,10 @@ def _arcs(self, runner, table, track, offset):
             )
 
         os.unlink(path)
-        return ("%08x" % v1, "%08x" % v2)
+        return "%08x" % v1, "%08x" % v2
 
-    def _foundOffset(self, device, offset):
+    @staticmethod
+    def _foundOffset(device, offset):
         print('\nRead offset of device is: %d.' % offset)
 
         info = drive.getDeviceInfo(device)

whipper/common/accurip.py

@@ -236,7 +236,7 @@ def print_report(result):
     """
     Print AccurateRip verification results.
     """
-    for i, track in enumerate(result.tracks):
+    for _, track in enumerate(result.tracks):
         status = 'rip NOT accurate'
         conf = '(not found)'
         db = 'notfound'

whipper/common/cache.py

@@ -93,23 +93,22 @@ def _unpickle(self, default=None):
         self.object = default
 
         if not self._path:
-            return None
+            return
 
         if not os.path.exists(self._path):
-            return None
+            return
 
         handle = open(self._path)
         import pickle
 
         try:
             self.object = pickle.load(handle)
             logger.debug('loaded persisted object from %r', self._path)
+        # FIXME: catching too general exception (Exception)
         except Exception as e:
-            # TODO: restrict kind of caught exceptions?
             # can fail for various reasons; in that case, pretend we didn't
             # load it
             logger.debug(e)
-            pass
 
     def delete(self):
         self.object = None
@@ -128,7 +127,7 @@ def __init__(self, path):
         try:
             os.makedirs(self.path)
         except OSError as e:
-            if e.errno != 17:  # FIXME
+            if e.errno != os.errno.EEXIST:  # FIXME: errno 17 is 'File Exists'
                 raise
 
     def _getPath(self, key):

whipper/common/checksum.py

@@ -47,7 +47,7 @@ def start(self, runner):
 
     def _crc32(self):
         if not self.is_wave:
-            fd, tmpf = tempfile.mkstemp()
+            _, tmpf = tempfile.mkstemp()
 
             try:
                 subprocess.check_call(['flac', '-d', self.path, '-fo', tmpf])

whipper/common/common.py

@@ -116,7 +116,7 @@ def formatTime(seconds, fractional=3):
     chunks = []
 
     if seconds < 0:
-        chunks.append(('-'))
+        chunks.append('-')
         seconds = -seconds
 
     hour = 60 * 60
@@ -271,23 +271,22 @@ def getRelativePath(targetPath, collectionPath):
     if targetDir == collectionDir:
         logger.debug('getRelativePath: target and collection in same dir')
         return os.path.basename(targetPath)
-    else:
-        rel = os.path.relpath(
-            targetDir + os.path.sep,
-            collectionDir + os.path.sep)
-        logger.debug('getRelativePath: target and collection '
-                     'in different dir, %r', rel)
-        return os.path.join(rel, os.path.basename(targetPath))
+    rel = os.path.relpath(
+        targetDir + os.path.sep,
+        collectionDir + os.path.sep)
+    logger.debug('getRelativePath: target and collection '
+                 'in different dir, %r', rel)
+    return os.path.join(rel, os.path.basename(targetPath))
 
 
 def validate_template(template, kind):
     """
     Raise exception if disc/track template includes invalid variables
     """
     if kind == 'disc':
-        matches = re.findall(r'%[^A,R,S,X,d,r,x,y]', template)
+        matches = re.findall(r'%[^ARSXdrxy]', template)
     elif kind == 'track':
-        matches = re.findall(r'%[^A,R,S,X,a,d,n,r,s,t,x,y]', template)
+        matches = re.findall(r'%[^ARSXadnrstxy]', template)
     if '%' in template and matches:
         raise ValueError(kind + ' template string contains invalid '
                          'variable(s): {}'.format(', '.join(matches)))

whipper/common/config.py

@@ -156,7 +156,6 @@ def _findOrCreateDriveSection(self, vendor, model, release):
             section = 'drive:' + urllib.quote('%s:%s:%s' % (
                 vendor, model, release))
             self._parser.add_section(section)
-            __pychecker__ = 'no-local'
             for key in ['vendor', 'model', 'release']:
                 self._parser.set(section, key, locals()[key].strip())
 

whipper/common/drive.py

@@ -66,6 +66,6 @@ def getDeviceInfo(path):
     except ImportError:
         return None
     device = cdio.Device(path)
-    ok, vendor, model, release = device.get_hwinfo()
+    _, vendor, model, release = device.get_hwinfo()
 
-    return (vendor, model, release)
+    return vendor, model, release

whipper/common/encode.py

@@ -60,7 +60,7 @@ def start(self, runner):
         self.schedule(0.0, self._flac_encode)
 
     def _flac_encode(self):
-        self.new_path = flac.encode(self.track_path, self.track_out_path)
+        flac.encode(self.track_path, self.track_out_path)
         self.stop()
 
 

whipper/common/mbngs.py

@@ -317,6 +317,4 @@ def musicbrainz(discid, country=None, record=False):
         return ret
     elif result.get('cdstub'):
         logger.debug('query returned cdstub: ignored')
-        return None
-    else:
-        return None
+    return None

whipper/common/path.py

@@ -45,7 +45,7 @@ def filter(self, path):
         def separators(path):
             # replace separators with a space-hyphen or hyphen
             path = re.sub(r'[:]', ' -', path, re.UNICODE)
-            path = re.sub(r'[\|]', '-', path, re.UNICODE)
+            path = re.sub(r'[|]', '-', path, re.UNICODE)
             return path
 
         # change all fancy single/double quotes to normal quotes
@@ -56,12 +56,12 @@ def separators(path):
 
         if self._special:
             path = separators(path)
-            path = re.sub(r'[\*\?&!\'\"\$\(\)`{}\[\]<>]',
+            path = re.sub(r'[*?&!\'\"$()`{}\[\]<>]',
                           '_', path, re.UNICODE)
 
         if self._fat:
             path = separators(path)
             # : and | already gone, but leave them here for reference
-            path = re.sub(r'[:\*\?"<>|"]', '_', path, re.UNICODE)
+            path = re.sub(r'[:*?"<>|]', '_', path, re.UNICODE)
 
         return path