-
Notifications
You must be signed in to change notification settings - Fork 431
Conversation
theromis
commented
May 18, 2016
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, 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! |
CLAs look good, thanks! |
I don't know Django well enough to review this. @dhermes or @jonparrott, might either or both of you look it over? |
@waprin Has been our go-to Django person |
Of course! @waprin, I apologize. |
LGTM. This PR drops support for 1.7 in favor of the development version, which seems reasonable given that 1.7 is EOL. Created #508 to make sure we explicitly document which versions of Django we support (1.8+ after this). @theromis I am curious if you are actually using FlowField or if the errors from it were just breaking things, because based on #494 I was planning on deleting it. |
@waprin checked for FlowField, don't see it in my code, only CredentialsField |
@waprin do you feel like any tests need to be updated for this? |
Yes, good point, Since we're still heavily mocking they wont' be the most useful tests but might as well keep it at 100%. @theromis let me know if you can write them, otherwise I will. |
@waprin I can write them, just give me template, from scratch is hard to do. |
https://github.com/google/oauth2client/blob/master/tests/contrib/test_django_orm.py is the current tests, just add methods to TestCredentialsField and TestFlowField that calls |
Closing this as it's obsoleted by #516 |
I was trying to follow the documentation here https://developers.google.com/api-client-library/python/guide/django, but it seems like it's out of date now. Are there plans to update it? |
@waprin do you want to take point on updating the docs? |