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

feat: pass a filename to oauth() #847

Merged

Conversation

bastienboutonnet
Copy link
Contributor

Aim

While load_credentials() allows for user's custom credentials path for service account credentials, oauth() doesn't but as discussed in #826 it looks like it is something several users would appreciate.

Given that oauth() simply wraps around load_credentials() it seems pretty straightforward to just allow oauth() to recieve an optional filename.

Solution

  • oauth() gets a an oprional credentials_filename argument which is then passed down to load_credentials() and defaults to DEFAULT_AUTHORIZED_USER_FILENAME so that the change is backward compatible with current documented usage.
  • Adds documentation snippets
  • Closes Allow gspread.oauth() to look for file in another directory #826

To Reviewer's Attention

  • I named the argument credentials_filename, however, I see that you folks generally just use filename, happy to keep things consistent here
  • I also have black turned on by default on my IDE which resulted in a formatting change across the entire file. I imagine that's not entirely welcome in this project so I'm happy to go ahead and correct that back. Unless you'd like to use black across your project and I'd be happy to help with that.
  • I couldn't find a unit test on the function I modified but let me know if I'm missing something

Copy link
Owner

@burnash burnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for the pull request.

Please revert to the original formatting as it makes the review much easier.

docs/oauth2.rst Outdated Show resolved Hide resolved
gspread/auth.py Outdated Show resolved Hide resolved
gspread/auth.py Outdated Show resolved Hide resolved
@bastienboutonnet
Copy link
Contributor Author

@burnash Thanks for reviewing and sorry for the delay in replying. I've changed back the formatting, sorry about that and made some of the changes you requested. I'm not sure about this comment though: #847 (comment)

And also, I wrote an answer to this one: #847 (comment) explaining the reason for my change.

Let me know if there's anything else that needs to change.

docs/oauth2.rst Outdated Show resolved Hide resolved
gspread/auth.py Outdated Show resolved Hide resolved
@bastienboutonnet
Copy link
Contributor Author

@burnash thanks for the additional pointers and good catches. I believe I've implemented the changes you requested.

@lavigne958
Copy link
Collaborator

Hi @bastienboutonnet I checked your PR, looks good to me ! 👍

Could you please squash your commits in order to have only a single commit that introduces the feature ? If you don't feel confident with this kind of git tricks tell me I'll guide you through it.

Then we could merge this relevant feature into master branch 😉

Add example usage in docstring

Update user doc and correct inaccuracy

revert formatting changes from black

rephrase docstring for authorized_user file

rename credentials_filename to authorized_user_filename

revert doc change

rename new parameter in doc and docstsings.
@bastienboutonnet bastienboutonnet force-pushed the feat/pass_filename_to_oauth branch from f8b7760 to bbf780d Compare May 4, 2021 16:08
@bastienboutonnet
Copy link
Contributor Author

Hi @lavigne958 thanks a lot for your review, I've just squashed so feel free to merge.

Thanks for asking. I generally just squash and merge contributor's branches on my OS project but this is a nice touch :)

@lavigne958 lavigne958 requested a review from burnash May 4, 2021 16:46
@lavigne958 lavigne958 merged commit 1567da1 into burnash:master May 4, 2021
@lavigne958
Copy link
Collaborator

Thank you @bastienboutonnet for your help, right I could do it this way. or cherry-pick every commits or... so many ways to do it with git 😆

@bastienboutonnet bastienboutonnet deleted the feat/pass_filename_to_oauth branch May 4, 2021 17:15
@bastienboutonnet
Copy link
Contributor Author

The pleasure was mine @lavigne958

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.

Allow gspread.oauth() to look for file in another directory
3 participants