Skip to content

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

Merged
merged 10 commits into from
Oct 18, 2020

Conversation

nwh
Copy link
Contributor

@nwh nwh commented Apr 9, 2020

Type of changes

  • Bug fix
  • New feature
  • Documentation / docstrings
  • Tests
  • Other

Checklist

  • I've run the latest black with default args on new code.
  • I've updated CHANGELOG.md and CONTRIBUTORS.md where appropriate.
  • I've added tests for new code.
  • I accept that @willmcgugan may be pedantic in the code review.

Description

Added readinto method for MemoryFS file objects. See #380 .

@nwh
Copy link
Contributor Author

nwh commented Apr 9, 2020

@willmcgugan I can add a test for this to tests_memoryfs.py if desired.

@nwh
Copy link
Contributor Author

nwh commented Apr 9, 2020

I tested manually via ipython:

In [2]: import fs                                                                                                       

In [3]: memfs = fs.open_fs("mem://")                                                                                    

In [4]: memfs.writebytes("file", b"here are some bytes")                                                                

In [5]: file = memfs.openbin("file", mode="rb")                                                                         

In [6]: b = bytearray(100)                                                                                              

In [7]: file.readinto(b)                                                                                                
Out[7]: 19

In [8]: b                                                                                                               
Out[8]: bytearray(b'here are some bytes\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00')

@lurch
Copy link
Contributor

lurch commented Apr 10, 2020

Not directly related to this PR, but should the next and readlines methods also be calling self.on_access() @willmcgugan ?

@willmcgugan
Copy link
Member

@lurch Yeah, I think so!

@willmcgugan
Copy link
Member

@nwh A test would be great. The test should ideally be in test.py so it will also test for readinto in other filesystems.

@willmcgugan
Copy link
Member

@nwh Would you mind adding those calls to on_access that @lurch pointed out?

@nwh
Copy link
Contributor Author

nwh commented Apr 10, 2020

@willmcgugan Also, _MemoryFile.read() checks if the file is open for reading. I could add that check to other read-like methods if you'd like.

@willmcgugan
Copy link
Member

@nwh Good idea, thanks. Don't forget to add yourself to contributors.

@nwh
Copy link
Contributor Author

nwh commented Apr 10, 2020

@willmcgugan looks like FTPFile also could use a readinto method. I could write one. My first thought is that FTPFile.readinto should simply use FTPFile.read and then put the bytes into the bytearray. How does this sound?

@nwh
Copy link
Contributor Author

nwh commented Apr 10, 2020

Also, I could change variable name b to buff or buffer for bytearray objects to satisfy the variable naming guidance.

@willmcgugan
Copy link
Member

@willmcgugan looks like FTPFile also could use a readinto method. I could write one. My first thought is that FTPFile.readinto should simply use FTPFile.read and then put the bytes into the bytearray. How does this sound?

Great. Seems like a reasonable approach. Sorry this is turning in to a bigger job that anticipated.

@nwh nwh changed the title Add readinto method for MemoryFS file objects Add readinto method for MemoryFS and FTPFS file objects Apr 10, 2020
fs/ftpfs.py Outdated
# type: (bytearray) -> int
data = self.read(len(buffer))
bytes_read = len(data)
buffer[: len(data)] = data
Copy link
Contributor

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 ?

Copy link
Contributor Author

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!

Copy link
Member

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!

Copy link
Contributor Author

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 nwh requested a review from willmcgugan April 17, 2020 00:17
@althonos althonos self-assigned this Sep 17, 2020
@althonos althonos added this to the v2.4.12 milestone Sep 17, 2020
@althonos althonos linked an issue Sep 17, 2020 that may be closed by this pull request
@nwh nwh requested a review from althonos October 15, 2020 19:10
@althonos
Copy link
Member

@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.

@nwh
Copy link
Contributor Author

nwh commented Oct 16, 2020

@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.

@althonos thanks, done

@althonos althonos merged commit c5d193b into PyFilesystem:master Oct 18, 2020
@althonos
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

missing readinto method for MemoryFS files?
4 participants