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

Remove @util.positional from the authorize() wrapper. #196

Merged
merged 2 commits into from
Jun 18, 2015

Conversation

eamonnmcmanus
Copy link
Contributor

Since the original method does not have a constraint on positional parameters,
its wrapper should not either. See
http://stackoverflow.com/questions/18126157/appengine-warning-during-python-app-update/30469210
for a case where this is producing annoying warnings.

@dhermes
Copy link
Contributor

dhermes commented Jun 17, 2015

Your change LGTM. The coverage decrease is just an unavoidable math fact caused by removing 1 line:

(a - 1) / (b - 1) < a / b <==> (a - 1) b < a (b - 1) <==> a < b

Regarding your comment


OK, I think github.com/google/oauth2client/pull/196 should fix this problem, assuming it is accepted and then propagated to the SDK.

I'm not sure how quickly a new version will be propagated to the SDK.

@dhermes
Copy link
Contributor

dhermes commented Jun 17, 2015

@nathanielmanistaatgoogle You are a go to merge, note that the method being monkey patched (ewwww I wish httplib2 didn't monkey patch an instance method on a live instance) does not have the util positional restriction.

PS I now have merge privileges but just wanted to make sure someone else saw.

@nathanielmanistaatgoogle
Copy link
Contributor

Thanks for the contribution!

(1) Please rewrite your commit message to conform to chris.beams.io/posts/git-commit/#seven-rules.

(2) You describe this change as a fix, so how about a regression test?

(3) How much digging did you do on this? In what commit was the original error made? What line of thinking or false assumption is the root cause of the mistake? Is there some larger lesson we should learn or action we should take beyond just deleting a single method decoration?

Since the original method does not have a constraint on positional
parameters, its wrapper should not either. See
http://stackoverflow.com/questions/18126157/appengine-warning-during-python-app-update/30469210
for a case where this is producing annoying warnings.
@eamonnmcmanus
Copy link
Contributor Author

(1) Updated the commit message.

(2) Added a regression test.

(3) To be honest, I think the cause of the mistake is using the @util.positional annotation at all. I don't see what advantage it confers, and it has certainly cost me and others a lot of time.

nathanielmanistaatgoogle added a commit that referenced this pull request Jun 18, 2015
Remove @util.positional from the authorize() wrapper.
@nathanielmanistaatgoogle nathanielmanistaatgoogle merged commit bb37fa5 into googleapis:master Jun 18, 2015
@dhermes
Copy link
Contributor

dhermes commented Jun 18, 2015

FYI that regression test has side-effects.

@nathanielmanistaatgoogle
Copy link
Contributor

That it does; I should have caught that.

@eamonnmcmanus
Copy link
Contributor Author

Yes sorry, I meant to add teardown logic. I'll send another PR with that.

eamonnmcmanus added a commit to eamonnmcmanus/oauth2client that referenced this pull request Jun 19, 2015
nathanielmanistaatgoogle added a commit that referenced this pull request Jun 23, 2015
Remove unwanted side-effect in test_oauth2client introduced by #196.
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