-
Notifications
You must be signed in to change notification settings - Fork 430
Adding _from_bytes helpers as a foil for _to_bytes. #272
Conversation
I should also FYI to @saaros that this is happening |
@@ -279,8 +280,7 @@ def new_from_json(cls, s): | |||
An instance of the subclass of Credentials that was serialized with | |||
to_json(). | |||
""" | |||
if isinstance(s, bytes): | |||
s = s.decode('utf-8') | |||
s = _from_bytes(s) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Looks mostly good; what else do you want to do here? |
New _from_bytes behavior warrants update to docstring. Also updated 1-letter or other non-descriptive variable names.
997279c
to
abca14e
Compare
@nathanielmanistaatgoogle Sent the new commit. I was looking for places that might need testing but kind of realized that the whole reason we test Also note I massively updated the
|
Looks good to me - anything remaining to be done here before merge? Anyone else who should review this? |
No and no. Merging. |
Adding _from_bytes helpers as a foil for _to_bytes.
@nathanielmanistaatgoogle This is the child of #157.
I labeled it don't merge because I essentially just looked around for
.decode
and tried to preserve a lot of the stuff from the other PR, but haven't written any new tests.Let's discuss (and I'll iterate) before merging.
UPDATE: I should note that I rebased the original PR if anyone wants to have a look: https://github.com/dhermes/oauth2client/tree/pr-157-rebased
Fixes #239 (
but no tests yettested byAssertionCredentialsTests.test_refresh_success_bytes
)