Handle files that do not exist

[jangop]

Closes #38

[sylikc]

interesting changes... I see your vision of where this goes. I thought about the check existence flag... looks like you did implement it exactly like I thought it had to be done...

Let me think a bit more about it. I started structuring some Errors... haven't checked anything in yet. I was on my way to learning how to return pre-bundled error messages, so it's consistent and not like

https://github.com/sylikc/pyexiftool/blob/715cfc818d93fca46a2079cb739cf4ca3cf3558e/exiftool/exiftool.py#L326

https://github.com/sylikc/pyexiftool/blob/715cfc818d93fca46a2079cb739cf4ca3cf3558e/exiftool/exiftool.py#L408

I am aiming for messages which are more comprehensive as to where it came from straight out of the text:

https://github.com/sylikc/pyexiftool/blob/715cfc818d93fca46a2079cb739cf4ca3cf3558e/exiftool/helper.py#L173

class ExifToolError(Exception):
    """
    Generic Base class for all ExifTool error classes
    """

class ProcessStateError(ExifToolError):
    """
    Base class for all errors related to the invalid state of `exiftool` subprocess
    """
    def __init__(self, message):
        self._message

class ExifToolIsRunning(ProcessStateError):
    """
    ExifTool is already running
    """

class ExifToolNotRunning(ProcessStateError):
    """
    ExifTool is not running
    """

class NoOutput(ExifToolError):
    """
    ExifTool did not return output
    """

[jangop]

Yes, that is the way most people would go about it (as far as I know). And simple messages are always a thing:

class GenericException(Exception):
    pass

class SpecificException(GenericException):
    pass

def main():
    raise SpecificException("Conrete Message")

if __name__ == '__main__':
    main()

This would yield __main__.SpecificException: Conrete Message with the following traceback:

Traceback (most recent call last):
  File "/home/jgoepfert/work/foobar/main.py", line 14, in <module>
    main()
  File "/home/jgoepfert/work/foobar/main.py", line 10, in main
    raise SpecificException("Conrete Message")
__main__.SpecificException: Conrete Message

This makes it really easy for users to handle exceptions coming from pyexiftool, and it is why exceptions more than anything else should be public (if they have any way of leaving the module) and documented. If one uses pyexiftool, they should be able to do something like:

try:
    tags = et.get_tags(...)
except exiftool.ExifToolError as e:
    logger.critical(f"That exiftool package has issues: {e}")
else:
    print(tags)

If you like, I can commit something like that.

[jangop]

I am aiming for messages which are more comprehensive as to where it came from straight out of the text:

And, and for this, it becomes increasingly difficult to keep track of correct names. If you rename the class or the method, you need to remember to update the messages as well. Which can lead to inconsistencies.

A better (“opinion warning”) solution are better-exceptions. I like to use loguru, which includes fully descriptive exceptions.

[sylikc]

And, and for this, it becomes increasingly difficult to keep track of correct names. If you rename the class or the method, you need to remember to update the messages as well. Which can lead to inconsistencies.

Ok, you're right, this is probably a bad idea... I can get the class name but not the method name...

self.__class__.__name__

but the method name is a whole 'nother mess. Ok, I can remove stuff like that

[sylikc]

Alright @jangop , I took a lot of the ideas and tests and stuff and coded it a bit differently.

I'll think more about the handling of non-existent files... it hit me that checking the status_code will reveal if invalid files were sent in (sometimes, not yet tested well). But in any case, I ripped out the -w detection code... as it was brittle to begin with (I did some tests and I broke it a few times over). I did write a new -w test to capture the problems it might cause

(Just a thought not going to do it) execute_json is so simple now that I almost want to move it into Helper... though since it's such core functionality probably not. The former function did all this complex encoding and decoding, and now it's so much simpler...

[sylikc]

what do you think about the naming of the exceptions? Are those good names? I think that's what held me back the longest... coming up with good names lol

[jangop]

what do you think about the naming of the exceptions? Are those good names? I think that's what held me back the longest... coming up with good names lol

There is some well-known meme about this somewhere. ☺️

Imho you did a great job. 👍

[sylikc]

now that v0.5.0 is out and documentation is all updated (kinda)... I am adding another flag to get_tags()

I'm adding check=False ... and for this PR, maybe it's validate=False ...

check is to mimic subprocess.check_output() which will raise an error if exit-status was non-zero

For validation, I was thinking checking both files AND tags. files for existence and tags for validity... to prevent a user from using get_tags to set tags... aka right now if you run get_tags("a.jpg", "tag=value"), it'll set the tag instead of getting it.

[sylikc]

do you think check should default to True ... now that I think about it, tags validation should always happen, regardless of the flag...

[sylikc]

I've made some very interesting changes to ExifToolHelper which helps check this on the return. By default, ExifToolHelper now checks the exit status code from ExifTool. This was a feature I added during the refactor, and didn't exist in the v0.4.x

So, if a non-existent file is passed, ExifToolHelper will raise an error. I've written a few test cases to verify the behavior

While it doesn't solve the problem on the front-end (this PR), it is another safety net that checks after execute()

[jangop]

do you think check should default to True ... now that I think about it, tags validation should always happen, regardless of the flag...

Absolutely. It makes use more intuitive and safer. If someone has a reason to bypass the test, they will surely look closely enough at the documentation to disable the check(s).

[sylikc]

Absolutely. It makes use more intuitive and safer. If someone has a reason to bypass the test, they will surely look closely enough at the documentation to disable the check(s).

definitely pretty interesting in my testing... i've made commits but didn't push anything. I've updated my own personal project to use the latest v0.5.2 and actually noticed that ExifToolExecuteError happens more often that I realized. So it's good to be a flag, definitely some legitimate uses.

I've tried to do the pattern matching for tags, and apparently it's a bit more complex than I originally thought... doing more testing before releasing.

[sylikc]

I think this is wayyy overcome by events... I think I made lots of changes that incorporates all these ideas earlier this year.. These were awesome fruitful discussions though and I wish it could hang around the repository forever!