sylikc / pyexiftool

PyExifTool (active PyPI project) - A Python library to communicate with an instance of Phil Harvey's ExifTool command-line application. Runs one process with special -stay_open flag, and pipes data to/from. Much more efficient than running a subprocess for each command!
Other
144 stars 17 forks source link

Reading tags from files that do not exist #38

Closed jangop closed 1 year ago

jangop commented 2 years ago

https://github.com/sylikc/pyexiftool/blob/3889b77abd4ef5463a17befb37cbb8af0f058ea0/tests/test_helper.py#L38 tests reading tags from a file that does not actually exist.

Looking at the source, I expect this will result in get_tags raising RuntimeError -- right?

Is this desirable? If yes, we should still define an appropriate exception, such as NoOutput:

class NoOutput(Exception):
    """
   ExifTool returned to output
   """
jangop commented 2 years ago

This also applies to https://github.com/sylikc/pyexiftool/blob/3889b77abd4ef5463a17befb37cbb8af0f058ea0/tests/test_helper.py#L46

sylikc commented 2 years ago

Is the recommended way to approach Exceptions to create our own exceptions? I've always just used the Built-In Exceptions, hehe... I use the RuntimeError as the generic catch-all

Maybe that's a clean way to go.. raise an Exception that is custom to ExifTool. ... what's the naming convention on these things?

sylikc commented 2 years ago

I saw this on PEP8 https://www.python.org/dev/peps/pep-0008/#exception-names ... I don't think no output is an error, but the stuff for ExifTool base class would be

jangop commented 2 years ago

Take the following as an example use case.

You are writing an application that takes in data sent in by users, processes it, and stores/distributes summaries. For this, metadata from images needs to be read. Since the application continuously runs in the background, proper exception handling is critical. If something goes wrong (and/or keeps going wrong), you must know exactly where the problem lies. A generic RuntimeError does not help in this regard. A specific exception from within pyexiftool does.

And you are correct about the naming.

sylikc commented 2 years ago

Ok, I'm convinced... but if I raise it from ExifToolHelper() it almost feels like then execute_json() should return that exception instead of None... for consistency? What do you think...

execute_json() currently returns None if the output of exilftool is nothing... if you use execute_json() for a WRITE operation, None is to be expected... or should we enforce using execute() for WRITE operations... and execute_json() must return a json? I wasn't sure when I made it return None awhile back

jangop commented 2 years ago

Ok, I'm convinced... but if I raise it from ExifToolHelper() it almost feels like then execute_json() should return that exception instead of None... for consistency? What do you think...

I agree. And then we can use that internally to return an empty list if it makes sense. We will want a consistent behavior. What if several files are provided, and some exist, while others don't? Raise an exception instead of returning anything? Or return empty lists / Nones together with data?

sylikc commented 2 years ago

What if several files are provided, and some exist, while others don't? Raise an exception instead of returning anything? Or return empty lists / Nones together with data?

That's trickier actually. Unless execute_json() or a wrapper function like get_metadata() does checking of parameters, there's no way of knowing whether some files don't exist. Exiftool by default ignores invalid parameters and files silently...

There's some code in ExifToolAlpha which someone made a PR which attempted checking, but it's so hacky and unreliable. I've tested it in some edge use cases and it fails dramatically.

sylikc commented 2 years ago

And then we can use that internally to return an empty list if it makes sense

hehe, does not really make sense. If I raise an exception it can return a value??

or the base class returns an exception and then Helper() interprets that to return []

sylikc commented 1 year ago

closed with PR being overcome by events (outdated)