search

smartfile / client-python

SmartFile API Client (Python).
http://app.smartfile.com/api/
MIT License
20 stars 7 forks source link

Python SDK update. Closes #3685 #7

Closed jnemery closed 8 years ago

jnemery commented 8 years ago

Implemented move, delete, upload, download methods.

Updated README documentation on how to perform these tasks.

Implemented testing for these tasks.

giampaolo commented 8 years ago

@btimby @jnemery @Ryanb58

I think this PR as it stands should not be merged and I also feel I don't agree with the general idea of providing so many additional APIs for something which is already very easy to do. 

    def delete(self, path):
        return self.post('/path/oper/remove', path=path)

    def upload(self, remote_filename, local_fileobj):
        arg = (remote_filename, local_fileobj)
        return self.post('/path/data/', file=arg)

    def download(self, remote_filename, local_fileobj):
        return shutil.copyfileobj(
            self.get('/path/data/', remote_filename), local_fileobj)

    def move(self, src_path, dst_path):
        return self.post('/path/oper/move/', src=src_path, dst=dst_path)

All of these methods are basically one-liners, meaning the fs operation can simply already be done by using self.post method. Adding an additional layer on top of something that straightforward is just not worth it. Instead, adding these to README in form of examples could be very useful. The only exception I see is the download() method. That can be worth being added because getting it right is more tricky, and also requires an additional lib which can be added as a dependency in setup.py. https://app.assembla.com/spaces/smartfile/tickets/3685/details?comment=979056983 In order to do that we need to fix the REST API first though.

Ryanb58 commented 8 years ago

@giampaolo Besides getting Jennifer assimilated with the SmartFile platform; the changes in this ticket are to include the most used endpoints as a part of the SDK. We believe that this will make it a bit easier on our less developer friendly people that end up having to use it. Also, it will provide those whom aren't familiar with using a REST API the ability to use the most common functionality of the platform without a large learning curve.

Therefore, I believe the changes to be sane, especially since they aren't really supposed to emulate all of the HTTP verbs anyways. If we were going to do that, I would just recommend to our clients the requests library and point them to our documentation, with the assumption that it will always be a knowledgeable developer working with them.

I have yet to review this PR though.

A good example would be an IT guy, not a coder trying to create a quick script to upload or download stuff.

Ryanb58 commented 8 years ago

Once the items that @itsthejoker mentioned are fixed/discussed, this PR can be merged.

Good job! :)

Ryanb58 commented 8 years ago

Also @jnemery note that I put the word Closes right before the ticket name. That way it automatically closes upon this PR being merged. If you don't want it to close, but just want to reference it you can use Re instead.