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

Confusing __str__() implementation for OAuthTokenResponse #639

Closed
rgov opened this issue Nov 3, 2022 · 1 comment · Fixed by #640
Closed

Confusing __str__() implementation for OAuthTokenResponse #639

rgov opened this issue Nov 3, 2022 · 1 comment · Fixed by #640
Assignees
Labels
enhancement New feature or improvement

Comments

@rgov
Copy link

rgov commented Nov 3, 2022

def __str__(self) -> str:
# Make printing responses more convenient by only showing the
# (typically important) token info
return json.dumps(self.by_resource_server, indent=2)

This makes it so that if you print() a OAuthTokenResponse instance, it shows you something different than if you actually tried to access members of the object.

E.g., it shows that there is a top-level transfer.api.globus.org key but in reality this is under other_tokens.

@sirosen
Copy link
Member

sirosen commented Nov 3, 2022

I tend to agree that it could be confusing. The __str__ here is meant to be informational and extract useful detail from the response, but it drops other fields and the classname.

I still prefer that __str__ shows by_resource_server rather than the other_tokens array. by_resource_server reshapes the data to fit the application model better and converts expires_in to expires_at_seconds (which is subtle but important, as you may want to store response data and reference it later).

I'm going to run an updated variant of this past our team and see what people think.

@sirosen sirosen self-assigned this Nov 3, 2022
@sirosen sirosen added the enhancement New feature or improvement label Nov 3, 2022
sirosen added a commit to sirosen/globus-sdk-python that referenced this issue Nov 3, 2022
This makes the str() of an oauth response object more verbose,
clarifying that `by_resource_server` is a field (not the set of
top-level keys) and including mention of `id_token`. The output is
also now more YAML-like, and not just an undecorated JSON dump.

In practice, the `id_token` can be very long, so this implementation
truncates it to the first 10 chars and notes it as `... (truncated)`.

No other fields are added to the stringifier as of yet.

resolves globus#639
@sirosen sirosen closed this as completed in 91c4f29 Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants