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

Credentials check before read #112

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

samjett247
Copy link

This pull request addresses the enhancement in #111 . Basically, if a user follows the quickstart instructions out of order by running python quickstart.py before downloading credentials.json in step 1 of the API quickstart, the program throws an uncaught error - "cannot find 'credentials.json'". I added an error catch for this in sheets/quickstart that directs the user to follow Step 1. If this solution looks okay I can make the changes to each of the other quickstarts.

@samjett247
Copy link
Author

This pull request addresses the enhancement in #111 . Basically, if a user follows the quickstart instructions out of order by running python quickstart.py before downloading credentials.json in step 1 of the API quickstart, the program throws an uncaught error - "cannot find 'credentials.json'". I added an error catch for this in sheets/quickstart that directs the user to follow Step 1. If this solution looks okay I can make the changes to each of the other quickstarts.

@erickoledadevrel @mcodik - Just wanted to ping you two as common contributors to this repo; Thoughts?

@erickoledadevrel
Copy link

@sqrrrl, @asrivas - What do you think? Should the could contain extra logic to detect and warn about common developer mistakes? If so, should the warnings mention specific steps in the quickstart instructions?

@sqrrrl
Copy link
Member

sqrrrl commented Jun 18, 2019

Maybe? I'm not opposed to a few extra guard clauses for common misconfigurations. Just want to make sure the wording in the message doesn't add any undue maintenance burden trying to keep things in sync. If we do go with this change, suggest doing it as a guard clause (if not found, print error & exit) before hand rather than an if/else. Makes it clearer that it is a prerequisite instead of an alternate path.

Related -- A lot of our quick start guides include troubleshooting sections. I'm curious why that wasn't sufficient in this case or if there's anything we can do to make that more useful.

@samjett247
Copy link
Author

Maybe? I'm not opposed to a few extra guard clauses for common misconfigurations. Just want to make sure the wording in the message doesn't add any undue maintenance burden trying to keep things in sync. If we do go with this change, suggest doing it as a guard clause (if not found, print error & exit) before hand rather than an if/else. Makes it clearer that it is a prerequisite instead of an alternate path.

Related -- A lot of our quick start guides include troubleshooting sections. I'm curious why that wasn't sufficient in this case or if there's anything we can do to make that more useful.

I didn't find anything regarding the error in the troubleshooting section. I think a section in the troubleshooting about it would be fine as well. Here was the specific error I get if it tries to read the credentials.json file w/o first creating the file via Step 1.

Traceback (most recent call last):
  File "quickstart.py", line 79, in <module>
    main()
  File "quickstart.py", line 49, in main
    'credentials.json', SCOPES)
  File "/usr/local/lib/python3.7/site-packages/google_auth_oauthlib/flow.py", line 180, in from_client_secrets_file
    with open(client_secrets_file, 'r') as json_file:
FileNotFoundError: [Errno 2] No such file or directory: 'credentials.json'

You could potentially add this error message to the troubleshooting section on the quickstart guide page, but I just thought catching and printing the error might be easier for users to quickly solve their error. But I see your point about not wanting to maintain sync; Maybe it would be easier to add the error to troubleshooting? Or to just print a link to the associated quickstart guide rather than the specific step? Lmk what you think @sqrrrl .

@samjett247
Copy link
Author

I changed the message so it shouldn't have any sync requirements, other than ensuring the URL for Quickstart doesn't change. I also changed the check to a guard clause and tried to make the program flow a little more easily understood through comments.

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