Repository navigation
Use BufferedReader instead of BytesIO in utils subpackage - #7981
jairideout wants to merge 1 commit into
Conversation
Addresses part of astropy#6652 by using `io.BufferedReader` instead of `io.BytesIO` in the `utils` subpackage. This change was only necessary in the `astropy.utils.data.get_readable_fileobj` function.
|
Hi there @jairideout 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃. Everything looks good from my point of view! 👍 If there are any issues with this message, please report them here. |
| return nbytes | ||
|
|
||
| stream = RawStream(fileobj) | ||
| fileobj = io.BufferedReader(stream) |
There was a problem hiding this comment.
I don't understand why the additional RawStream layer is needed, instead of using directly BufferedReader ?
There was a problem hiding this comment.
The constructor for io.BufferedReader requires an object that's a subclass (or at least a duck type) of io.RawIOBase.
In this part of the codepath for get_readable_fileobj(), fileobj is only guaranteed to have a .read() method, so an io.RawIOBase subclass is required to wrap fileobj in an appropriate interface for io.BufferedReader.
Codecov Report
@@ Coverage Diff @@
## master #7981 +/- ##
==========================================
+ Coverage 86.86% 86.86% +<.01%
==========================================
Files 377 377
Lines 57174 57189 +15
Branches 1016 1016
==========================================
+ Hits 49663 49678 +15
Misses 6927 6927
Partials 584 584
Continue to review full report at Codecov.
|
3 similar comments
Codecov Report
@@ Coverage Diff @@
## master #7981 +/- ##
==========================================
+ Coverage 86.86% 86.86% +<.01%
==========================================
Files 377 377
Lines 57174 57189 +15
Branches 1016 1016
==========================================
+ Hits 49663 49678 +15
Misses 6927 6927
Partials 584 584
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #7981 +/- ##
==========================================
+ Coverage 86.86% 86.86% +<.01%
==========================================
Files 377 377
Lines 57174 57189 +15
Branches 1016 1016
==========================================
+ Hits 49663 49678 +15
Misses 6927 6927
Partials 584 584
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #7981 +/- ##
==========================================
+ Coverage 86.86% 86.86% +<.01%
==========================================
Files 377 377
Lines 57174 57189 +15
Branches 1016 1016
==========================================
+ Hits 49663 49678 +15
Misses 6927 6927
Partials 584 584
Continue to review full report at Codecov.
|
|
I am proposing a simpler approach at #8634. |
|
@pllim #8634 is indeed simpler than this PR (just wrap Just wanted to note the differences between our solutions -- thanks! |
|
@jairideout , gotcha. However, I am not sure anymore that we even need either PR, as the current code is not causing any problem AFAIK and the intent of the original comment was lost with time. 🤷♀️ |
|
Sounds good! Feel free to close this PR if you decide it's not worth including due to the additional complexity 👍 |
|
I had a look but could not find a real use case for this while running the tests. Sorry for the useless work, but I think that we should close, and wait for a real need to consider using this, given the additional complexity. |
|
I concur with @saimn . Thanks for the work anyway, @jairideout . It is much appreciated and we hope you continue to contribute! |
Addresses part of #6652 by using
io.BufferedReaderinstead ofio.BytesIOin theutilssubpackage. This change was only necessary in theastropy.utils.data.get_readable_fileobjfunction.