planetarypy / pvl

Python implementation of PVL (Parameter Value Language)
BSD 3-Clause "New" or "Revised" License
19 stars 19 forks source link

Provide an improved error message when Py3 users fail to open a file as a binary. #6

Closed godber closed 4 years ago

godber commented 9 years ago

My PR https://github.com/planetarypy/pvl/pull/5 failed in python 3. The test was to loop over all input label files, load() them, and test to see if they are instances of Label. You can see the travis results here:

https://travis-ci.org/planetarypy/pvl/builds/64809958

godber commented 9 years ago

This is not actually a problem, see comment here:

https://github.com/planetarypy/pvl/pull/5#discussion_r31392516

wtolson commented 9 years ago

This is a problem I think @michaelaye ran into as well. Even with documentation, I think this could be a tricky bug for people. The parser should either raise an error that is more instructive or should try to decode the stream. I'm leaning towards a better error message but open to thoughts.

godber commented 9 years ago

Could the code test to see if the file was opened in the incorrect mode and raise an error based on that? I am not sure I understand the error well enough. Is it specifically "In python3 the file must be opened in binary mode to work"?

wtolson commented 9 years ago

The problem is python2 opens files by default in binary mode where as python3 opens files in unicode mode. It would be simple enough to test just by seeing if the stream returns bytes or unicode strings.

wtolson commented 9 years ago

The pvl decoder expects a stream of bytes to work but the stream could be wrapped decoded on the fly similar to how its buffered now.

wtolson commented 9 years ago

My only concern with decoding on the fly is it could cause even trickier bugs for people if the encoding is incorrect or malformed.

godber commented 9 years ago

Seems like we should raise an error if we are seeing a stream of unicode then. Thats invalid input right? Raise an error with a good error message. I am going to go look at some Astropy error handling.

godber commented 9 years ago

OK, emulating everything AstroPy does is a rabbit hole I don't want to go down yet. So test for the stream type and if its not byte then raise TypeError(msg) where msg = "Byte string expected, in Python 3 open file in binary mode."??

michaelaye commented 9 years ago

I'm a bit confused as I am working with PDS3 labels successfully. Can someone post the exact failure mode? I can comment though that I had to remove the build directory with a 'rm -rf' before I got things working with a python setup.py install after @wtolson updated for Python 3 compatibility.

Edit: I just saw that this issue is not for Pysis. I only have experience with that.

godber commented 9 years ago

Hi @michaelaye They work fine if you load() the file directly. If you open(..., 'r') the file in Python 3 then, as Trevor says, the decoder received a unicode stream instead of a byte stream, and fails. So it does work fine if you give it the right input. The discussion now is about how to properly inform the user when they encounter the error. Which I think is fleshed out. I will change the title of this issue to properly reflect the change.

godber commented 9 years ago

@michaelaye incidentally, shoot me an email at godber@gmail.com so we can talk about PlanetaryPy.

michaelaye commented 9 years ago

ah, that reminds me of my issue with using CubeFile on a filename without using open. I think I also would advocate to implement automatic incoming filetype checking inside the CubeFile constructor for user convenience.

rbeyer commented 4 years ago

I believe this issue is addressed by #38 and can be closed.

michaelaye commented 4 years ago

hm, i find it a bit problematic to close issue simply on believe status. Can we point to specific things that confirm that issues are being solved?

rbeyer commented 4 years ago

This Issue is obsoleted by the functionality in #38 which will now take all of the following types as the first argument to pvl.load() which is exhibited in tests/test_pvl.py

In short, this issue was based on the assumption that a user must open a file in binary mode to pass to pvl.load() in Python 3 (because it didn't matter in Python 2) given the old architecture. Now that this library is completely Python 3, and the architecture has been revamped, we can handle all of the above types and don't need to emit an error message, because this isn't a problem anymore.