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

Issue #142: CredentialField Python 3 fix #316

Merged

Conversation

apragacz
Copy link
Contributor

@apragacz apragacz commented Oct 1, 2015

This PR should fix issue #142.

Changes

  • in oauth2client.django_orm.CredentialsField:
    • Python 2/3 metaclass support
    • using get_prep_value instead of get_db_prep_value
    • added value_to_string method
    • explicit conversions to text/bytes types in get_prep_value, to_python (required by Python 3)
  • in test_django_orm tests:
    • added explicit conversion to bytes so the tests can work on Python 3

@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@apragacz
Copy link
Contributor Author

apragacz commented Oct 1, 2015

I signed it!

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Oct 1, 2015
@dhermes
Copy link
Contributor

dhermes commented Oct 1, 2015

@nathanielmanistaatgoogle Here are the docs for smart_bytes and smart_text:

https://docs.djangoproject.com/en/1.8/ref/utils/#django.utils.encoding.smart_bytes (behaves like our _helpers._to_bytes)
https://docs.djangoproject.com/en/1.8/ref/utils/#django.utils.encoding.smart_text (behaves like our _helpers._from_bytes)

@dhermes
Copy link
Contributor

dhermes commented Oct 1, 2015

Also worth noting this fixes the Python3 issues with metaclasses, previously attempted in #227 and it looks like this fixes #168 (in addition to #142, where the conversation with @szopu started).

@nathanielmanistaatgoogle Should we also have him fix class FlowField(models.Field) to work in Python3.


def get_db_prep_value(self, value, connection, prepared=False):

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@apragacz apragacz force-pushed the django-credentials-field-py3-fix branch 2 times, most recently from 8e56fda to 35fcc85 Compare October 4, 2015 22:50
@apragacz apragacz force-pushed the django-credentials-field-py3-fix branch from 35fcc85 to b6bb23f Compare October 5, 2015 08:42
@apragacz
Copy link
Contributor Author

apragacz commented Oct 5, 2015

@dhermes Done. Added the docstring & additonal tests. I decided that low-level testing of value_to_string should be enough.

@dhermes
Copy link
Contributor

dhermes commented Oct 5, 2015

I wanted to make sure we don't introduce newly untested code paths.

We did not. After this PR, the only remaining untested parts are

  • the django_orm.Storage class (100% untested, even constructor)
  • the FlowField.to_python() method (has two uncovered branches)
  • the FlowField.get_db_prep_value() method (has one uncovered branch)

All of these were untested before (and not part of this PR), so that's a good thing!


You can verify (as I did with these links to the untested lines).

Before this PR, the untested code paths are

https://github.com/google/oauth2client/blob/af6d3eb8067d5c95fb59af6fb62a192e6c7fa16b/oauth2client/django_orm.py#L46
https://github.com/google/oauth2client/blob/af6d3eb8067d5c95fb59af6fb62a192e6c7fa16b/oauth2client/django_orm.py#L48
https://github.com/google/oauth2client/blob/af6d3eb8067d5c95fb59af6fb62a192e6c7fa16b/oauth2client/django_orm.py#L53
https://github.com/google/oauth2client/blob/af6d3eb8067d5c95fb59af6fb62a192e6c7fa16b/oauth2client/django_orm.py#L71
https://github.com/google/oauth2client/blob/af6d3eb8067d5c95fb59af6fb62a192e6c7fa16b/oauth2client/django_orm.py#L73
https://github.com/google/oauth2client/blob/af6d3eb8067d5c95fb59af6fb62a192e6c7fa16b/oauth2client/django_orm.py#L78
https://github.com/google/oauth2client/blob/af6d3eb8067d5c95fb59af6fb62a192e6c7fa16b/oauth2client/django_orm.py#L101-L104
https://github.com/google/oauth2client/blob/af6d3eb8067d5c95fb59af6fb62a192e6c7fa16b/oauth2client/django_orm.py#L112-L120
https://github.com/google/oauth2client/blob/af6d3eb8067d5c95fb59af6fb62a192e6c7fa16b/oauth2client/django_orm.py#L131-L140
https://github.com/google/oauth2client/blob/af6d3eb8067d5c95fb59af6fb62a192e6c7fa16b/oauth2client/django_orm.py#L145-L146

Now they are

https://github.com/szopu/oauth2client/blob/b6bb23f0559c48bfb229de09142f01ee612ae3d4/oauth2client/django_orm.py#L83
https://github.com/szopu/oauth2client/blob/b6bb23f0559c48bfb229de09142f01ee612ae3d4/oauth2client/django_orm.py#L85
https://github.com/szopu/oauth2client/blob/b6bb23f0559c48bfb229de09142f01ee612ae3d4/oauth2client/django_orm.py#L90
https://github.com/szopu/oauth2client/blob/b6bb23f0559c48bfb229de09142f01ee612ae3d4/oauth2client/django_orm.py#L113-L116
https://github.com/szopu/oauth2client/blob/b6bb23f0559c48bfb229de09142f01ee612ae3d4/oauth2client/django_orm.py#L124-L132
https://github.com/szopu/oauth2client/blob/b6bb23f0559c48bfb229de09142f01ee612ae3d4/oauth2client/django_orm.py#L143-L152
https://github.com/szopu/oauth2client/blob/b6bb23f0559c48bfb229de09142f01ee612ae3d4/oauth2client/django_orm.py#L157-L158

self.assertEqual(_to_bytes(prep_value), self.pickle)


class TestCredentialsFieldViaModel(unittest.TestCase):

This comment was marked as spam.

@dhermes
Copy link
Contributor

dhermes commented Oct 6, 2015

This LGTM

@nathanielmanistaatgoogle are we a go to merge this?


After merging, someone (I can do it) needs to update django_orm.FlowField to also be Python3 compatible (modeled after this PR) and then add tests for it (that PR might as well also add tests for django_orm.Storage).

@nathanielmanistaatgoogle
Copy link
Contributor

Looks mergeable to me; thank you for leading review!

dhermes added a commit that referenced this pull request Oct 6, 2015
@dhermes dhermes merged commit 232e24c into googleapis:master Oct 6, 2015
@dhermes
Copy link
Contributor

dhermes commented Oct 6, 2015

Thanks @szopu !

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.

4 participants