Refactored exiftool.py

CHANGELOG.md

@@ -4,6 +4,18 @@ All notable changes to this project will be documented in this file. Dates are d
 
 Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog).
 
+#### [v0.36.7](https://github.com/RhetTbull/osxphotos/compare/v0.36.6...v0.36.7)
+
+> 4 November 2020
+
+- Implemented context manager for ExifTool, closes #250 [`#250`](https://github.com/RhetTbull/osxphotos/issues/250)
+
+#### [v0.36.6](https://github.com/RhetTbull/osxphotos/compare/v0.36.5...v0.36.6)
+
+> 2 November 2020
+
+- Fix for issue #39 [`c7c5320`](https://github.com/RhetTbull/osxphotos/commit/c7c5320587e31070b55cc8c7e74f30b0f9e61379)
+
 #### [v0.36.5](https://github.com/RhetTbull/osxphotos/compare/v0.36.4...v0.36.5)
 
 > 1 November 2020

README.md

@@ -1859,6 +1859,7 @@ The following substitutions are availabe for use with `PhotoInfo.render_template
 |{modified.month}|Month name in user's locale of the file modification time|
 |{modified.mon}|Month abbreviation in the user's locale of the file modification time|
 |{modified.dd}|2-digit day of the month (zero padded) of the file modification time|
+|{modified.dow}|Day of week in user's locale of the photo modification time|
 |{modified.doy}|3-digit day of year (e.g Julian day) of file modification time, starting from 1 (zero padded)|
 |{modified.hour}|2-digit hour of the file modification time|
 |{modified.min}|2-digit minute of the file modification time|

osxphotos/_version.py

@@ -1,4 +1,4 @@
 """ version info """
 
-__version__ = "0.36.7"
+__version__ = "0.36.8"
 

osxphotos/exiftool.py

@@ -100,7 +100,7 @@ class _ExifToolProc:
             ],
             stdin=subprocess.PIPE,
             stdout=subprocess.PIPE,
-            stderr=subprocess.DEVNULL,
+            stderr=subprocess.STDOUT,
         )
         self._process_running = True
 
@@ -133,13 +133,19 @@ class ExifTool:
     """ Basic exiftool interface for reading and writing EXIF tags """
 
     def __init__(self, filepath, exiftool=None, overwrite=True):
-        """ Return ExifTool object
-            file: path to image file
-            exiftool: path to exiftool, if not specified will look in path
-            overwrite: if True, will overwrite image file without creating backup, default=False """
+        """ Create ExifTool object
+
+        Args:
+                file: path to image file
+                exiftool: path to exiftool, if not specified will look in path
+                overwrite: if True, will overwrite image file without creating backup, default=False
+        Returns:
+            ExifTool instance
+        """
         self.file = filepath
         self.overwrite = overwrite
         self.data = {}
+        self.error = None
         # if running as a context manager, self._context_mgr will be True
         self._context_mgr = False
         self._exiftoolproc = _ExifToolProc(exiftool=exiftool)
@@ -147,8 +153,18 @@ class ExifTool:
         self._read_exif()
 
     def setvalue(self, tag, value):
-        """ Set tag to value(s) 
-            if value is None, will delete tag """
+        """ Set tag to value(s); if value is None, will delete tag
+
+        Args:
+            tag: str; name of tag to set
+            value: str; value to set tag to
+        
+        Returns:
+            True if success otherwise False
+            
+            If error generated by exiftool, returns False and sets self.error to error string
+            If called in context manager, returns True (execution is delayed until exiting context manager)
+        """
 
         if value is None:
             value = ""
@@ -157,19 +173,32 @@ class ExifTool:
             command.append("-overwrite_original")
         if self._context_mgr:
             self._commands.extend(command)
+            return True
         else:
-            self.run_commands(*command)
+            _, self.error = self.run_commands(*command)
+            return self.error is None
 
     def addvalues(self, tag, *values):
         """ Add one or more value(s) to tag
             If more than one value is passed, each value will be added to the tag
-            Notes: exiftool may add duplicate values for some tags so the caller must ensure
-                   the values being added are not already in the EXIF data
-                   For some tags, such as IPTC:Keywords, this will add a new value to the list of keywords, 
-                   but for others, such as EXIF:ISO, this will literally add a value to the existing value.
-                   It's up to the caller to know what exiftool will do for each tag
-                   If setvalue called before addvalues, exiftool does not appear to add duplicates, 
-                   but if addvalues called without first calling setvalue, exiftool will add duplicate values
+
+        Args:
+            tag: str; tag to set
+            *values: str; one or more values to set
+        
+        Returns:
+            True if success otherwise False
+            
+            If error generated by exiftool, returns False and sets self.error to error string
+            If called in context manager, returns True (execution is delayed until exiting context manager)
+            
+        Notes: exiftool may add duplicate values for some tags so the caller must ensure
+               the values being added are not already in the EXIF data
+               For some tags, such as IPTC:Keywords, this will add a new value to the list of keywords, 
+               but for others, such as EXIF:ISO, this will literally add a value to the existing value.
+               It's up to the caller to know what exiftool will do for each tag
+               If setvalue called before addvalues, exiftool does not appear to add duplicates, 
+               but if addvalues called without first calling setvalue, exiftool will add duplicate values
         """
         if not values:
             raise ValueError("Must pass at least one value")
@@ -185,14 +214,26 @@ class ExifTool:
 
         if self._context_mgr:
             self._commands.extend(command)
+            return True
         else:
-            self.run_commands(*command)
+            _, self.error = self.run_commands(*command)
+            return self.error is None
 
     def run_commands(self, *commands, no_file=False):
-        """ run commands in the exiftool process and return result 
-            no_file: (bool) do not pass the filename to exiftool (default=False)
-                     by default, all commands will be run against self.file
-                     use no_file=True to run a command without passing the filename """
+        """ Run commands in the exiftool process and return result.
+
+        Args:
+                *commands: exiftool commands to run
+                no_file: (bool) do not pass the filename to exiftool (default=False)
+                        by default, all commands will be run against self.file
+                        use no_file=True to run a command without passing the filename
+        Returns:
+            (output, errror)
+            output: bytes is containing output of exiftool commands
+            error: if exiftool generated an error, bytes containing error string otherwise None
+
+        Note: Also sets self.error if error generated.
+        """
         if not (hasattr(self, "_process") and self._process):
             raise ValueError("exiftool process is not running")
 
@@ -218,9 +259,16 @@ class ExifTool:
 
         # read the output
         output = b""
+        error = b""
         while EXIFTOOL_STAYOPEN_EOF not in str(output):
-            output += self._process.stdout.readline().strip()
-        return output[:-EXIFTOOL_STAYOPEN_EOF_LEN]
+            line = self._process.stdout.readline()
+            if line.startswith(b"Warning"):
+                error += line
+            else:
+                output += line.strip()
+        error = None if error == b"" else error
+        self.error = error
+        return output[:-EXIFTOOL_STAYOPEN_EOF_LEN], error
 
     @property
     def pid(self):
@@ -230,14 +278,14 @@ class ExifTool:
     @property
     def version(self):
         """ returns exiftool version """
-        ver = self.run_commands("-ver", no_file=True)
+        ver, _ = self.run_commands("-ver", no_file=True)
         return ver.decode("utf-8")
 
     def asdict(self):
         """ return dictionary of all EXIF tags and values from exiftool 
             returns empty dict if no tags
         """
-        json_str = self.run_commands("-json")
+        json_str, _ = self.run_commands("-json")
         if json_str:
             exifdict = json.loads(json_str)
             return exifdict[0]
@@ -246,7 +294,8 @@ class ExifTool:
 
     def json(self):
         """ returns JSON string containing all EXIF tags and values from exiftool """
-        return self.run_commands("-json")
+        json, _ = self.run_commands("-json")
+        return json
 
     def _read_exif(self):
         """ read exif data from file """
@@ -265,4 +314,4 @@ class ExifTool:
         if exc_type:
             return False
         elif self._commands:
-            self.run_commands(*self._commands)
+            _, self.error = self.run_commands(*self._commands)

tests/test_exiftool.py

@@ -103,10 +103,28 @@ def test_setvalue_1():
 
     exif = osxphotos.exiftool.ExifTool(tempfile)
     exif.setvalue("IPTC:Keywords", "test")
+    assert not exif.error
+
     exif._read_exif()
     assert exif.data["IPTC:Keywords"] == "test"
 
 
+def test_setvalue_error():
+    # test setting illegal tag value generates error
+    import os.path
+    import tempfile
+    import osxphotos.exiftool
+    from osxphotos.fileutil import FileUtil
+
+    tempdir = tempfile.TemporaryDirectory(prefix="osxphotos_")
+    tempfile = os.path.join(tempdir.name, os.path.basename(TEST_FILE_ONE_KEYWORD))
+    FileUtil.copy(TEST_FILE_ONE_KEYWORD, tempfile)
+
+    exif = osxphotos.exiftool.ExifTool(tempfile)
+    exif.setvalue("IPTC:Foo", "test")
+    assert exif.error
+
+
 def test_setvalue_context_manager():
     # test setting a tag value as context manager
     import os.path
@@ -124,6 +142,8 @@ def test_setvalue_context_manager():
         exif.setvalue("XMP:Title", "title")
         exif.setvalue("XMP:Subject", "subject")
 
+    assert exif.error is None
+
     exif2 = osxphotos.exiftool.ExifTool(tempfile)
     exif2._read_exif()
     assert sorted(exif2.data["IPTC:Keywords"]) == ["test1", "test2"]
@@ -131,6 +151,22 @@ def test_setvalue_context_manager():
     assert exif2.data["XMP:Subject"] == "subject"
 
 
+def test_setvalue_context_manager_error():
+    # test setting a tag value as context manager when error generated
+    import os.path
+    import tempfile
+    import osxphotos.exiftool
+    from osxphotos.fileutil import FileUtil
+
+    tempdir = tempfile.TemporaryDirectory(prefix="osxphotos_")
+    tempfile = os.path.join(tempdir.name, os.path.basename(TEST_FILE_ONE_KEYWORD))
+    FileUtil.copy(TEST_FILE_ONE_KEYWORD, tempfile)
+
+    with osxphotos.exiftool.ExifTool(tempfile) as exif:
+        exif.setvalue("Foo:Bar", "test1")
+    assert exif.error
+
+
 def test_clear_value():
     # test clearing a tag value
     import os.path

tests/test_export_catalina_10_15_7.py

@@ -744,7 +744,7 @@ def test_xmp_sidecar_is_valid(tmp_path):
     xmp_file = tmp_path / XMP_FILENAME
     assert xmp_file.is_file()
     exiftool = ExifTool(str(xmp_file))
-    output = exiftool.run_commands("-validate", "-warning")
+    output, _ = exiftool.run_commands("-validate", "-warning")
     assert output == b"[ExifTool]      Validate                        : 0 0 0"