-
-
Notifications
You must be signed in to change notification settings - Fork 177
Add readinto method for MemoryFS and FTPFS file objects #381
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@willmcgugan I can add a test for this to |
I tested manually via
|
Not directly related to this PR, but should the |
@lurch Yeah, I think so! |
@nwh A test would be great. The test should ideally be in |
@willmcgugan Also, |
@nwh Good idea, thanks. Don't forget to add yourself to contributors. |
@willmcgugan looks like |
Also, I could change variable name |
Great. Seems like a reasonable approach. Sorry this is turning in to a bigger job that anticipated. |
fs/ftpfs.py
Outdated
# type: (bytearray) -> int | ||
data = self.read(len(buffer)) | ||
bytes_read = len(data) | ||
buffer[: len(data)] = data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason this isn't buffer[:bytes_read] = data
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, good point. I just copied the implementation from fs.iotools.RawWrapper.readinto
. The implementation for fs.iotools.RawWrapper.readinto1
is also the same. I could update all three locations if requested. Cheers!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nwh : could you please update this (as well as the code in RawWrapper.readinto
)? I'll merge the PR right after. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@althonos done -- however, there is conflict in the change log. How would you like that handled?
@nwh : in order to fix the conflics, best would be if you could rebase against the current master branch, then update this PR by force-pushing. |
Thanks! |
Type of changes
Checklist
Description
Added
readinto
method forMemoryFS
file objects. See #380 .