Merge lp://staging/~mterry/ubuntuone-couch/extra-headers into lp://staging/ubuntuone-couch

Proposed by Michael Terry
Status: Merged
Approved by: dobey
Approved revision: 9
Merged at revision: 8
Proposed branch: lp://staging/~mterry/ubuntuone-couch/extra-headers
Merge into: lp://staging/ubuntuone-couch
Diff against target: 22 lines (+4/-2)
1 file modified
ubuntuone/couch/auth.py (+4/-2)
To merge this branch: bzr merge lp://staging/~mterry/ubuntuone-couch/extra-headers
Reviewer Review Type Date Requested Status
dobey (community) Approve
Eric Casteleijn (community) Approve
Review via email: mp+61648@code.staging.launchpad.net

Commit message

Allow extra headers in auth.request()

To post a comment you must log in.
Revision history for this message
Eric Casteleijn (thisfred) wrote :

Looks good, except this is a bad idea:

+ token_secret=None, headers={}):

As default values in Python should as a rule not be mutable types. Default values are evaluated at import time, making this empty dictionary a 'global' variable, so that in this case the oauth headers for one call to request() will pollute the dictionary for any subsequent calls.

Please change it to:

+ token_secret=None, headers=None):

and then at the beginning of the function implementation do:

headers = headers or {}

Also a test case would be awesome, but if you cannot think of one, I can look at that.

review: Needs Fixing
9. By Michael Terry

use None instead of {} in signature

Revision history for this message
Michael Terry (mterry) wrote :

OK, updated to use your None method.

Could you please look at a test? I looked briefly and would copy the test_request_plaintext method and just add a test header like "Foo": "Bar", but I wasn't sure how to test whether the outgoing request was what I expected.

Revision history for this message
Eric Casteleijn (thisfred) wrote :

Looks great now, I'll think of a test and add it in a separate branch.

review: Approve
Revision history for this message
dobey (dobey) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches