Ape / samsungctl

Remote control Samsung televisions via a TCP/IP connection
MIT License
713 stars 191 forks source link

Replace FileNotFoundError with IOError for Python 2 compatibility #30

Closed fhempy closed 7 years ago

fhempy commented 7 years ago

Remove FileNotFoundError which is not supported in python2.

Ape commented 7 years ago

I don't like the catch-all. Which exception is thrown on Python 2 if the file is not found?

Are these all the changes required to support Python 2?

Ape commented 7 years ago

I appreciate that you are contributing to the project, and I might accept commits that improve the compatibility with different Python versions. However, I'm not planning to support Python 2.

Supporting Python 2 would require to always test commits and releases with Python 2. It might also require making the code more complex in some case. I will accept commits for Python 2 support, but I cannot guarantee that Python 2 will work always in the future.

fhempy commented 7 years ago

Hi, thanks for your quick reply.

I can understand, that you don't want to maintain support for 2 versions. I tested it with python 2.7 and it works with just this one change.

I found this about FileNotFoundError in Python 2: http://stackoverflow.com/questions/26745283/how-do-i-import-filenotfounderror-from-python-3 Neverthelss, I would recommend that the tool works even if other errors occure. You could also print an error message of the exception and continue with default config.

As I plan to use your tool with python 2, I might also provide pull requests if it doesn't work with python 2 in future. Thank you very much for your great work!

Ape commented 7 years ago

I'm okay with the catch-all if you move the os.path.join call outside the try block.

fhempy commented 7 years ago

What do you mean by moving the os.path.join outside of the try block? That part is the only reason for the try block.

Ape commented 7 years ago

Isn't it the open call that raises the exception? I mean this:

...
for directory in directories:
    path = os.path.join(directory, "samsungctl.conf")
    try:
        config_file = open(path)
    except:
        ...
Ape commented 7 years ago

Would except OSError work with both Python 2 and Python 3?

Ape commented 7 years ago

Also, I thought about this case. I think it's better to only catch "file not found". If there is a file, but it cannot be opened, then it's better just to fail and print an error message explaining why that file cannot be opened. It might be very confusing if we just skip the file silently.

fhempy commented 7 years ago

Ok, now it handles only "file not found" and raises an exception for everything else.

Tested with python 2.7 and works as expected. Would be great if you could merge it. Thanks!

Ape commented 7 years ago

Thanks, looks good to me! Please squash the commits and perhaps name it something like "Replace FileNotFoundError with IOError for Python 2 compatibility".

fhempy commented 7 years ago

Squashed commits and renamed.

Ape commented 7 years ago

Thanks!