Skip to content

Persist parameters with auto_paging_iter()#222

Merged
brandur merged 1 commit into
masterfrom
ob-fix-221
Apr 11, 2016
Merged

Persist parameters with auto_paging_iter()#222
brandur merged 1 commit into
masterfrom
ob-fix-221

Conversation

@olivierbellone
Copy link
Copy Markdown
Contributor

This PR fixes #221: auto_paging_iter() should now persist parameters.

r? @brandur
cc @stripe/api-libraries

Comment thread stripe/resource.py
response, api_key = requestor.request('get', url, params)
return convert_to_stripe_object(response, api_key, stripe_account)
stripe_object = convert_to_stripe_object(response, api_key, stripe_account)
stripe_object._retrieve_params = params
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I'm never a fan of assigning to "private" instance variables, I believe this is the best way to fix the current bug.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was wondering about this one too.

Here's what PEP-8 has to say about the leading underscore:

_single_leading_underscore: weak "internal use" indicator. E.g. from M import * does not import objects whose name starts with an underscore.

In this case, _retrieve_params should be considered a property of StripeObject. Because ListObject (a descendant of StripeObject) already accesses it, we could consider its visibility to be currently "protected" (based on the definition from other OO languages). ListableAPIResource is also a descendant of StripeObject so we're not really modifying that visibility from what it was before.

The idea of protected visibility is also in-line with PEP-8's idea of internal use, so I think we're okay on this here.

@olivierbellone olivierbellone force-pushed the ob-fix-221 branch 2 times, most recently from 4886224 to bf5a5da Compare April 11, 2016 18:18
@brandur
Copy link
Copy Markdown
Contributor

brandur commented Apr 11, 2016

@olivierbellone Thanks a lot for tackling this fix man! Also, you measurably improved the quality of those tests, so thank-you for taking a look at that too.

Going to get this shipped!

@brandur brandur merged commit a55e118 into master Apr 11, 2016
@brandur brandur deleted the ob-fix-221 branch April 11, 2016 19:32
@brandur
Copy link
Copy Markdown
Contributor

brandur commented Apr 11, 2016

Released as 1.32.1.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

auto_paging_iter() does not persist params

4 participants