Skip to content
This repository has been archived by the owner on Nov 5, 2019. It is now read-only.

Unifying all conversions to bytes. #250

Merged

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Aug 10, 2015

Also making sure the _urlsafe_b64encode returns bytes and updating dependent code with the change in return type.

NOTE: This doesn't address the opposite direction, i.e. a.decode('utf-8') where is a bytes (or str) object.

Raises:
ValueError if the value could not be converted to bytes.
"""
result = value

This comment was marked as spam.

This comment was marked as spam.

@nathanielmanistaatgoogle
Copy link
Contributor

As they are used throughout this commit what's the difference between "isinstance(value, bytes)" and "isinstance(value, six.binary_type)"? I imagine the distinction is subtle and tricky?

@dhermes
Copy link
Contributor Author

dhermes commented Aug 12, 2015

AFAIK the only difference is that bytes is an alias for str in Py2.6 and Py2.7 but it doesn't exist in Py2.5, so six.py just says:

if PY3:
    binary_type = bytes
else:
    binary_type = str

@dhermes dhermes force-pushed the fix-bytes-in-_do_revoke branch from 737825a to e6ff48f Compare August 14, 2015 02:38
@dhermes
Copy link
Contributor Author

dhermes commented Aug 14, 2015

@nathanielmanistaatgoogle I rebased after some of the changes made it un-mergeable

Things that remain:

  • Use of ternary expression
  • Using .format instead of % (varname,).
  • Renaming _to_bytes to to_bytes (this will make it a public symbol in modules it gets used in)

Seems this will mostly cover #136 and #157

@nathanielmanistaatgoogle
Copy link
Contributor

  • Yes to conditional expression
  • No, not unless I persuaded you, and I don't think I did because I think you persuaded me
  • Oh crap, that's because this codebase has a feel-free-to-import-elements-of-modules policy instead of import-only-modules-not-elements-of-modules policy. Oof, leave the underscore for now in that case.

Also making sure the _urlsafe_b64encode returns bytes
and updating dependent code with the change in
return type.
@dhermes dhermes force-pushed the fix-bytes-in-_do_revoke branch from e6ff48f to 5c570a8 Compare August 14, 2015 15:46
@dhermes
Copy link
Contributor Author

dhermes commented Aug 14, 2015

@nathanielmanistaatgoogle I folded the ternary change into the original PR commit.

It's more common in Python to import the objects you need rather than the namespaces you need. That's just a Google rule. When unit testing, it's easier (maybe only marginally so, maybe a lot?) to monkey patch the objects in the module under test rather than tracking down the namespace which has that object and then monkey patching that other module.

@dhermes
Copy link
Contributor Author

dhermes commented Aug 14, 2015

I learned a little git-fu for anyone who wants to learn:

For #136:

$ # Visit: https://api.github.com/repos/google/oauth2client/pulls/136/commits
$ # parent in MASTER: 3c02a812165004aeb18cc89e10639694bf2d3b0f
$ git branch pr-136 3c02a812165004aeb18cc89e10639694bf2d3b0f
$ git checkout pr-136
$ wget https://patch-diff.githubusercontent.com/raw/google/oauth2client/pull/136.patch
$ git am 136.patch

For #157 :

$ # Visit: https://api.github.com/repos/google/oauth2client/pulls/157/commits
$ # parent in MASTER: 0a6241c792f0c833f2f176292b0c7ea5623eabfd
$ git branch pr-157 0a6241c792f0c833f2f176292b0c7ea5623eabfd
$ git checkout pr-157
$ wget https://patch-diff.githubusercontent.com/raw/google/oauth2client/pull/157.patch
$ git am 157.patch

Now I will try to rebase them onto master and see how it goes.

@dhermes dhermes mentioned this pull request Aug 26, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants