-
Notifications
You must be signed in to change notification settings - Fork 53
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
Enable moving files to folder IDs as well as names #48
base: master
Are you sure you want to change the base?
Conversation
… using the folder name
I was unable to see what Better Code Hub complained about this PR as I do not have the required permissions. Please advise on any changes that must be made. Thanks! |
Hi! I'm so sorry I hadn't gotten to this, I must have missed the e-mail notification about it (and I've been busy with a new job so I hadn't been checking the repo regularly). I'll try to take a look this week, and thanks so much for your contribution! |
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.
Thanks for your contribution!! I definitely agree that adding the ability to use the folder ID is useful, and this PR seems to address that. I made a couple suggestions and would be happy to merge after those are addressed.
@@ -384,7 +384,7 @@ def create_folder(self, path, parents=True): | |||
self.refresh_directories() | |||
return parent | |||
|
|||
def move_file(self, file_id, path, create=False): | |||
def move_file(self, file_id, path, id_string, create=False): |
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.
id_string
here is the folder id, right? if so, I think folder_id
would be more descriptive (id_string
indicates it's an ID and a string, but it's not clear an id for what).
Since folder_id
becomes an alternative to path
, we should make both parameters optional with a default of None
and a check that at least one of them is provided.
@@ -1012,7 +1012,7 @@ def list_permissions(self): | |||
""" | |||
return self.client.list_permissions(self.spread.id) | |||
|
|||
def move(self, path="/", create=True): | |||
def move(self, path="/", id_string=None, create=True): |
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.
same as above, should be named folder_id
Hi, I found this PR and this functionality would be quite useful to me. I made the changes that you suggested but I'm not really sure how to submit them to this existing PR but I also didn't want to just create a new one and add clutter. What would be the best/preferred way to handle this is? |
Simple change which fixed an issue I was having (and which is currently mentioned in issue #46 ) where a folder cannot be found by name when using the move command but whose ID can be located with the gspread_pandas client or by examining the URL. This enables moving a file to a folder (using either client.move_file() or spread.move()) using its id, rather than its name. This is done with the id_string argument. When not explicitly using the id_string argument, it behavies identically to previous behaviour.
This is my first time contributing to open source, please forgive any mistakes in the process. Hope this helps!