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

Switch from pickle to jsonpickle #614

Merged
merged 1 commit into from Aug 12, 2016

Conversation

Projects
None yet
4 participants
@waprin
Contributor

waprin commented Aug 11, 2016

Addresses #594. As the reporter mentions the PickleSerializer is not recommended. The flow object can't be encoded with json.dumps, we could add a custom serializer but this jsonpickle library seems to work and is super simple so figured I would try that.

/cc @huwshimi

@googlebot googlebot added the cla: yes label Aug 11, 2016

@@ -14,6 +14,7 @@ basedeps = mock>=1.3.0
deps = {[testenv]basedeps}
django
keyring
jsonpickle

This comment has been minimized.

@dhermes

dhermes Aug 11, 2016

Contributor

Will django users have this dependency? Should we add some extras to setup.py for people that want the django dependencies?

@dhermes

dhermes Aug 11, 2016

Contributor

Will django users have this dependency? Should we add some extras to setup.py for people that want the django dependencies?

This comment has been minimized.

@waprin

waprin Aug 11, 2016

Contributor

they won't have any particular reason to have it. what do you have in mind? not totally sure how extra dependencies work.

@waprin

waprin Aug 11, 2016

Contributor

they won't have any particular reason to have it. what do you have in mind? not totally sure how extra dependencies work.

This comment has been minimized.

@dhermes

dhermes Aug 11, 2016

Contributor

See an example of extras. You'd just give a list like

DJANGO_EXTRAS = ('django', 'jsonpickle')

setup(
    ...
    extras_require={'django': DJANGO_EXTRAS},
    ...
)

and then people could install via

pip install oauth2client[django]
@dhermes

dhermes Aug 11, 2016

Contributor

See an example of extras. You'd just give a list like

DJANGO_EXTRAS = ('django', 'jsonpickle')

setup(
    ...
    extras_require={'django': DJANGO_EXTRAS},
    ...
)

and then people could install via

pip install oauth2client[django]

This comment has been minimized.

@waprin

waprin Aug 11, 2016

Contributor

I do think making the contrib stuff more optional, as well as improving tox so it runs the tests on the django version/python version matrix would be good. But I think given this is a bugfix maybe hold that change for a separate PR?

@waprin

waprin Aug 11, 2016

Contributor

I do think making the contrib stuff more optional, as well as improving tox so it runs the tests on the django version/python version matrix would be good. But I think given this is a bugfix maybe hold that change for a separate PR?

This comment has been minimized.

@dhermes

dhermes Aug 11, 2016

Contributor

👍 to a separate PR

@dhermes

dhermes Aug 11, 2016

Contributor

👍 to a separate PR

@mock.patch('oauth2client.contrib.django_util.views.pickle')
def test_callback_works(self, pickle):
@mock.patch('oauth2client.contrib.django_util.views.jsonpickle')
def test_callback_works(self, jsonpickle_mock):

This comment has been minimized.

@dhermes

dhermes Aug 11, 2016

Contributor

Thanks for the rename, still no verification of how the mock was used though

@dhermes

dhermes Aug 11, 2016

Contributor

Thanks for the rename, still no verification of how the mock was used though

This comment has been minimized.

@waprin

waprin Aug 11, 2016

Contributor

done

@waprin

waprin Aug 11, 2016

Contributor

done

@@ -180,9 +181,10 @@ def test_callback_works(self, pickle):
self.assertEqual(
response.status_code, django.http.HttpResponseRedirect.status_code)
self.assertEqual(response['Location'], self.RETURN_URL)
jsonpickle_mock.decode.assert_called_once_with(pickled_flow)

This comment has been minimized.

@dhermes

dhermes Aug 11, 2016

Contributor

Was encode called (other than your jsonpickle_mock.encode(flow) above)? Also, what is the point of that? Wouldn't pickled_flow = object() work just fine?

@dhermes

dhermes Aug 11, 2016

Contributor

Was encode called (other than your jsonpickle_mock.encode(flow) above)? Also, what is the point of that? Wouldn't pickled_flow = object() work just fine?

This comment has been minimized.

@waprin

waprin Aug 11, 2016

Contributor

Done, yes now that you mention it setting a value to the return of a mock you just created is kind of silly. I'm guessing there was an iteration of the test that at some point made sense and then not-thoughtful refactors made it otherwise, thanks.

@waprin

waprin Aug 11, 2016

Contributor

Done, yes now that you mention it setting a value to the return of a mock you just created is kind of silly. I'm guessing there was an iteration of the test that at some point made sense and then not-thoughtful refactors made it otherwise, thanks.

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Aug 11, 2016

Contributor

LGTM. I'll let someone else have a pass first before merging

Contributor

dhermes commented Aug 11, 2016

LGTM. I'll let someone else have a pass first before merging

@waprin

This comment has been minimized.

Show comment
Hide comment
@waprin

waprin Aug 12, 2016

Contributor

@jonparrott hey, i just met you, and this is crazy, but LGTM, so merge me maybe?

Contributor

waprin commented Aug 12, 2016

@jonparrott hey, i just met you, and this is crazy, but LGTM, so merge me maybe?

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Aug 12, 2016

Member

:D I never get to use any Carly Rae Jepsen gifs.

carly

Member

theacodes commented Aug 12, 2016

:D I never get to use any Carly Rae Jepsen gifs.

carly

@theacodes theacodes merged commit c9b4b07 into googleapis:master Aug 12, 2016

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details

@waprin waprin referenced this pull request Sep 20, 2016

Merged

Django samples #636

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment