-
Notifications
You must be signed in to change notification settings - Fork 904
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
base: master
Are you sure you want to change the base?
Credentials check before read #112
Conversation
@erickoledadevrel @mcodik - Just wanted to ping you two as common contributors to this repo; Thoughts? |
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.
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 . |
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. |
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 downloadingcredentials.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.