-
Notifications
You must be signed in to change notification settings - Fork 219
Change downscope_token() to return a TokenResponse object #237
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
Conversation
|
Verified that @anthonywee has signed the CLA. Thanks for the pull request! |
- Add a TokenResponse object, which is a subclass of BaseAPIJSONObject. - OAuth2.downscope_token() now returns a TokenResponse object, which allows users to get other fields of the response, such as 'expires_in'.
4e0a956 to
e90a22a
Compare
boxsdk/auth/oauth2.py
Outdated
|
|
||
| access_token, _ = self._send_token_request_without_storing_tokens(data, access_token, expect_refresh_token=False) | ||
| return access_token | ||
| token_response = self.execute_token_request(data, access_token, expect_refresh_token=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.
Just return from here.
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.
done
boxsdk/auth/oauth2.py
Outdated
| self._access_token, self._refresh_token = access_token, refresh_token | ||
|
|
||
| def _send_token_request_without_storing_tokens(self, data, access_token, expect_refresh_token=True): | ||
| def execute_token_request(self, data, access_token, expect_refresh_token=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.
I think we should keep this private for now.
Not that I think it should never be public. But until we know that we definitely need this in the public API, I don't think we should clutter the API and add more backwards-compatibility restrictions for ourselves.
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.
sure, done
Jeff-Meadows
left a comment
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.
don't forget to bump the version if you'd like this to be released
allows users to get other fields of the response, such as
'expires_in'.