Skip to content
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

added extract method #127

Merged
merged 8 commits into from
Jan 3, 2018
Merged

added extract method #127

merged 8 commits into from
Jan 3, 2018

Conversation

willmcgugan
Copy link
Member

Adds an extract method to base, to help with PyFilesystem/s3fs#18

It should allow 'downloading' of files without the need for a temp file. The default is a simple file copy loop.

Open to suggestions re naming. Is extract appropriate?

@althonos

@coveralls
Copy link

coveralls commented Dec 28, 2017

Coverage Status

Coverage increased (+4.0e-05%) to 99.951% when pulling 6dbda8b on extract into 84c0f7d on master.

@coveralls
Copy link

coveralls commented Dec 28, 2017

Coverage Status

Coverage increased (+4.0e-05%) to 99.951% when pulling 801c78d on extract into 84c0f7d on master.

@coveralls
Copy link

coveralls commented Dec 28, 2017

Coverage Status

Coverage increased (+4.0e-05%) to 99.951% when pulling 801c78d on extract into 84c0f7d on master.

@althonos
Copy link
Member

althonos commented Dec 29, 2017

The code looks good (just had a quick glance), and the use case seems generic enough to be reused in other filesystems.

About the name, it could be confusing: tarfile method extract is actually closer to an open method (return a read-only file handle). The closest method of the standard library is probably copyfileobj of the shutil module. So maybe copyfileto or dumpto could be candidates as well (using the to suffix to emphasize the FS is writing to a file handle).

@willmcgugan
Copy link
Member Author

Good point re the name. I've noticed a pattern that probably dictates the name. We have setbytes, settext, setfile, and getbytes, gettext. But no getfile.

I think this extract method is exactly what I would expect getfile to be. i.e. the logical inverse of setfile.

@coveralls
Copy link

coveralls commented Dec 31, 2017

Coverage Status

Coverage increased (+4.0e-05%) to 99.951% when pulling 9ea6066 on extract into 84c0f7d on master.

@coveralls
Copy link

coveralls commented Dec 31, 2017

Coverage Status

Coverage increased (+0.001%) to 99.952% when pulling dd3a536 on extract into 84c0f7d on master.

@coveralls
Copy link

coveralls commented Dec 31, 2017

Coverage Status

Coverage increased (+5.0e-05%) to 99.951% when pulling dd3a536 on extract into 84c0f7d on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+5.0e-05%) to 99.951% when pulling dd3a536 on extract into 84c0f7d on master.

@willmcgugan willmcgugan merged commit bf0dbd8 into master Jan 3, 2018
@willmcgugan willmcgugan deleted the extract branch January 3, 2018 17:14
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.

None yet

3 participants