Repository navigation
Clarify file signature detection details in astropy.utils.data.get_readable_fileobj() - #12641
Conversation
This comment has been minimized.
This comment has been minimized.
|
Okay, I take it back. I see this failure in another PR too. Something must have just changed upstream... |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Might have to just pin it (#12643) until we can figure out astropy/pytest-astropy-header#41 |
cc96af0 to
016381a
Compare
|
@saimn , do you want to have a look by feature freeze or should we move the milestone? |
|
Sorry @eerovaher it's too short to do a proper review before the freeze, especially since there was a previous attempt at doing a similar change (#7981). I don't remember all the details but the gain of doing this change was not clear at that time (and the code change in #7981 was a bit more complicated). |
|
Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then. In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary. If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock. If you believe I commented on this pull request incorrectly, please report this here. |
|
I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks! If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here. |
|
Oooops I think this fell through the cracks. @eerovaher , if you are still interested, we can re-open but you will have to rebase and force push to reset the stale clock. Please let me know. I apologize for any inconvenience caused. |
eerovaher
removed
Close?
016381a to
46d42ee
Compare
|
The comments seem to indicate that the code that would be replaced is overly complicated because of backwards compatibility with Python 2, and the fact that this pull request passes CI tests confirms it. I think it would be good to merge this, so I have reopened the pull request and rebased the commits. |
|
Thanks! |
|
I don't see anything obviously wrong with the diff but I restarted the dev job to make sure it matches the failures on main. If not, they might be related to your change here. I will also kick off the cron jobs, just in case. |
|
I don't think the dev job failures are related. |
| fileobj.seek(0) | ||
| else: | ||
| fileobj = io.BufferedReader(fileobj) | ||
| signature = fileobj.peek(4) |
There was a problem hiding this comment.
But if a file-like object has no .seek method, it will also most probably not have a .seekable method and this will crash ?
There was a problem hiding this comment.
If an object does not have seek() and seekable() methods then it is not an instance of io.IOBase.
| raise OSError | ||
|
|
||
| def seekable(self): | ||
| return False |
There was a problem hiding this comment.
The test seems quite different from the initial one, which was a simple object with just a read method. What's the goal here ?
There was a problem hiding this comment.
The goal of the changes here was to ensure that FakeStream would be a subclass of io.RawIOBase.
|
Is there a specific use case for this change ? While looking at this in the past it wasn't clear if any change was needed/useful. |
|
For me the main motivation for this pull request was to remove Python 2 compatibility code, and the updated code always works as long as the file object is an instance of |
|
@eerovaher , do you want to do one more rebase to get clean CI? @aarchiba , are you interested to take a look since you refactored a bunch of this before? |
46d42ee to
0184b77
Compare
|
WCSAxes failures are unrelated. |
|
@saimn was going to have another look. Thanks! |
Right, the code and the test make it clear that the goal is support objects that are not |
Previously a code comment explained Python 2 compatibility requirements. However, in astropyGH-12641 it was decided that even though Python 2 is no longer supported, compatibility concerns are still relevant . [ci skip]
0184b77 to
5065ed0
Compare
eerovaher
changed the title
astropy.utils.data.get_readable_fileobj()astropy.utils.data.get_readable_fileobj()
Previously a code comment explained Python 2 compatibility requirements. However, in astropyGH-12641 it was decided that even though Python 2 is no longer supported, compatibility concerns are still relevant . [ci skip]
Description
Currently
astropy.utils.data.get_readable_fileobj()checks the file signature of a non-random access file-like object (i.e. one that does not have aseek()method) by reading in all its contents to access the first few bytes. It would be better to wrap such a file-like object in aio.BufferedReader. A comment in the code lists a few Python 2 reasons why that can't be done, but Python 2 is no longer supported so those reasons are not valid anymore.EDIT: The discussion below came to the conclusion that compatibility issues are still relevant despite Python 2 not being supported anymore, so this pull request only updates the code comment so that it wouldn't be misleading.
Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
Extra CIlabel.no-changelog-entry-neededlabel. If this is a manual backport, use theskip-changelog-checkslabel unless special changelog handling is necessary.astropy-botcheck might be missing; do not let the green checkmark fool you.backport-X.Y.xlabel(s) before merge.