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

experimental api refresh #248

Merged
merged 19 commits into from Jan 1, 2019

Conversation

@willmcgugan
Copy link
Member

commented Dec 29, 2018

API Refresh (WIP)

I've had at the back of my mind for a while that some of the api methods could be better named. I think it was @althonos that first pointed out some inconsistencies in the naming. This PR is an experimental branch where I can test potential method name changes. I'll elicit feedback before I merge any of this.

It will be a breaking change, so it would require a major version increment, according to semver. Hopefully everyone has learned to pin their requirements!

Added the old methods as aliases with deprecation warnings. So it isn't a breaking change yet.

The changes are mostly in the helper methods, which most filesystems don't implement. I wouldn't expect many third party filesystems to have to make changes other than updating requirements. Applications could be affected though.

Please see comments in base.py

@@ -550,7 +550,7 @@ def exclude_file(patterns, info):
iter_info = itertools.islice(iter_info, start, end)
return iter_info

def getbytes(self, path):
def readbytes(self, path):

This comment has been minimized.

Copy link
@willmcgugan

willmcgugan Dec 29, 2018

Author Member

I think read/write is more intuitive that get/set in filesystem parlance. Although, writing to a file doesn't necessarily imply truncating it first. I'm hoping that the fact there is appendbytes and friends makes it clear what this does.

@@ -568,7 +568,7 @@ def getbytes(self, path):
contents = read_file.read()
return contents

def getfile(self, path, file, chunk_size=None, **options):
def download(self, path, file, chunk_size=None, **options):

This comment has been minimized.

Copy link
@willmcgugan

willmcgugan Dec 29, 2018

Author Member

getfile has always felt awkward as it doesn't return a file, like gettext returns text.

From a network filesystem download makes total sense, it may not be quite so obvious for other filesystems, but I think it actually fits. It suggests (to me at least) that the data is being retrieved from the filesystem.

fs/base.py Outdated
@@ -1244,7 +1244,7 @@ def setbytes(self, path, contents):
with closing(self.open(path, mode="wb")) as write_file:
write_file.write(contents)

def setbinfile(self, path, file):
def upload(self, path, file):

This comment has been minimized.

Copy link
@willmcgugan

willmcgugan Dec 29, 2018

Author Member

Like download, I think upload better describes what this does than the clumsy setbinfile.

@willmcgugan

This comment has been minimized.

Copy link
Member Author

commented Dec 29, 2018

@althonos @zopyx @geoffjukes @birnbaum @lurch Looking for feedback on this one. Not in a hurry to merge. Please see description above.

@zopyx

This comment has been minimized.

Copy link
Member

commented Dec 30, 2018

I dont care much about the name changes. Perhaps you should provide aliases and deprecation warnings for older code.

@willmcgugan

This comment has been minimized.

Copy link
Member Author

commented Dec 30, 2018

@zopyx Good idea re deprecation warnings. Then it wouldn't require a major version change straight away.

@coveralls

This comment has been minimized.

Copy link

commented Dec 30, 2018

Coverage Status

Coverage decreased (-1.2%) to 98.491% when pulling 309d61e on api-refresh into e5e85ad on master.

willmcgugan added 8 commits Dec 30, 2018
@althonos

This comment has been minimized.

Copy link
Member

commented Dec 30, 2018

I'm happy with all the changes: the more concerning issue to me was the inconsistent names of setfile, setbinfile and getfile. Having upload/download feels a lot better.

fs/ftpfs.py Outdated
self,
path, # type: Text
namespaces=None # type: Optional[Container[Text]]
self, path, namespaces=None # type: Text # type: Optional[Container[Text]]

This comment has been minimized.

Copy link
@willmcgugan

willmcgugan Dec 30, 2018

Author Member

Black broke mymy comments. Todo

willmcgugan added 8 commits Dec 31, 2018

@willmcgugan willmcgugan merged commit d6cf403 into master Jan 1, 2019

0 of 3 checks passed

Scrutinizer Running
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@willmcgugan willmcgugan deleted the api-refresh branch Jan 1, 2019

@geoffjukes

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2019

Thanks @willmcgugan and @althonos for updating the other packages (s3 & Archive) for this too. It made this a simple update for me (only a handful of references). 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.