Make more requested changes for PR 3236

- Moved several variable assignments outside of try blocks
- Added and clarified various comments and docstrings
- Modified the command loop in `get_im_version()` to a slightly more
readable approach
- `get_im_version()` now returns None when ImageMagick is unavailable
- Updated `ArtResizer._check_method` to handle our new returns in a way
that is more readable
- Fixed an issue where `get_im_version()` could crash if the regex
search failed to find a match

beets/util/artresizer.py

@@ -82,7 +82,7 @@ def pil_resize(maxwidth, path_in, path_out=None):
 
 def im_resize(maxwidth, path_in, path_out=None):
     """Resize using ImageMagick's ``magick`` tool
-    (or fall back to ``convert`` for older versions.)
+    (or fall back to ``convert`` for older versions).
     Return the output path of resized image.
     """
     path_out = path_out or temp_file_for(path_in)
@@ -92,17 +92,18 @@ def im_resize(maxwidth, path_in, path_out=None):
     # "-resize WIDTHx>" shrinks images with the width larger
     # than the given width while maintaining the aspect ratio
     # with regards to the height.
-    try:
-        cmd = ArtResizer.shared.im_convert_cmd + \
-            [util.syspath(path_in, prefix=False),
-                '-resize', '{0}x>'.format(maxwidth),
-                util.syspath(path_out, prefix=False)]
+    cmd = ArtResizer.shared.im_convert_cmd + \
+        [util.syspath(path_in, prefix=False),
+            '-resize', '{0}x>'.format(maxwidth),
+            util.syspath(path_out, prefix=False)]
 
+    try:
         util.command_output(cmd)
     except subprocess.CalledProcessError:
         log.warning(u'artresizer: IM convert failed for {0}',
                     util.displayable_path(path_in))
         return path_in
+
     return path_out
 
 
@@ -123,10 +124,10 @@ def pil_getsize(path_in):
 
 
 def im_getsize(path_in):
-    try:
-        cmd = ArtResizer.shared.im_identify_cmd + \
-            ['-format', '%w %h', util.syspath(path_in, prefix=False)]
+    cmd = ArtResizer.shared.im_identify_cmd + \
+        ['-format', '%w %h', util.syspath(path_in, prefix=False)]
 
+    try:
         out = util.command_output(cmd)
     except subprocess.CalledProcessError as exc:
         log.warning(u'ImageMagick size query failed')
@@ -176,6 +177,9 @@ def __init__(self):
         log.debug(u"artresizer: method is {0}", self.method)
         self.can_compare = self._can_compare()
 
+        # Use ImageMagick's magick binary when it's available. If it's
+        # not, fall back to the older, separate convert and identify
+        # commands.
         if self.method[0] == IMAGEMAGICK:
             self.im_legacy = self.method[2]
             if self.im_legacy:
@@ -230,13 +234,14 @@ def _can_compare(self):
 
     @staticmethod
     def _check_method():
-        """Return a tuple indicating an available method and its version."""
-        try:
-            version, legacy = get_im_version()
-            if version > (0, 0, 0):
-                return IMAGEMAGICK, version, legacy
-        except TypeError:
-            pass
+        """Return a tuple indicating an available method and its version.
+        If the method is ImageMagick, also return a bool indicating whether to
+        use the `magick` binary or legacy utils (`convert`, `identify`, etc.)
+        """
+        version = get_im_version()
+        if version:
+            version, legacy = version
+            return IMAGEMAGICK, version, legacy
 
         version = get_pil_version()
         if version:
@@ -246,40 +251,36 @@ def _check_method():
 
 
 def get_im_version():
-    """Return Image Magick version or None if it is unavailable
-    Try invoking ImageMagick's "magick". If "magick" is unavailable,
-    as with older versions, fall back to "convert"
+    """Return ImageMagick version/legacy-flag pair or None if the check fails.
 
-    Our iterator will be non-zero when the first command fails, and will
-    be returned in a tuple along with the version.
+    Try invoking ImageMagick's `magick` binary first, then `convert` if
+    `magick` is unavailable.
     """
-    cmd_names = (['magick'],
-                 ['convert'])
-    for i, cmd_name in enumerate(cmd_names):
+    for cmd_name, legacy in ((['magick'], False), (['convert'], True)):
+        cmd = cmd_name + ['--version']
 
         try:
-            cmd = cmd_name + ['--version']
             out = util.command_output(cmd)
 
             if b'imagemagick' in out.lower():
                 pattern = br".+ (\d+)\.(\d+)\.(\d+).*"
                 match = re.search(pattern, out)
-                version = (int(match.group(1)),
-                           int(match.group(2)),
-                           int(match.group(3)))
-                legacy = bool(i)
                 if match:
-                    return (version, legacy)
+                    version = (int(match.group(1)),
+                               int(match.group(2)),
+                               int(match.group(3)))
+                    return version, legacy
 
         except (subprocess.CalledProcessError, OSError) as exc:
             log.debug(u'ImageMagick version check failed: {}', exc)
 
-        return ((0,), None)
+        return None
 
 
 def get_pil_version():
-    """Return Image Magick version or None if it is unavailable
-    Try importing PIL."""
+    """Return Pillow version or None if it is unavailable
+    Try importing PIL.
+    """
     try:
         __import__('PIL', fromlist=[str('Image')])
         return (0,)