-
Notifications
You must be signed in to change notification settings - Fork 85
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
Support automatic authentication #300
Conversation
👈 Launch a binder notebook on this branch for commit 73b9af6 I will automatically update this comment whenever this PR is modified 👈 Launch a binder notebook on this branch for commit 641eafc 👈 Launch a binder notebook on this branch for commit 335ae89 👈 Launch a binder notebook on this branch for commit 1a20f87 👈 Launch a binder notebook on this branch for commit 1cba817 |
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.
I like this! I wonder if we should have a repr method to show the users if they are authenticated and if they are in us-west-2 to incorporate what @battistowx suggested in #231
I think that's a reasonable thing to add. Where would the repr method go? Maybe a new FWIW, while I like the repr idea, my sense is it's probably separate from the automatic login logic here (though let me know if you think otherwise) |
I'm inclined to agree here! |
I think this is my last thought: how should this change impact the documentation? |
I went ahead and updated the authentication section of the docs -- let me know what you think |
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.
Looks great to me! I don't have the same level of familiarity with the library as @betolink so you may want a second opinion ;)
Thanks for the review @MattF-NSIDC! I'm going to go ahead and merge this in. Though happy to handle review comments from others like @betolink in a follow-up |
💯 |
Closes #299, closes #283