Skip to content
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

Split resource.py #351

Merged
merged 2 commits into from
Oct 20, 2017
Merged

Split resource.py #351

merged 2 commits into from
Oct 20, 2017

Conversation

ob-stripe
Copy link
Contributor

Splits resource.py into separate files. At 1200+ lines long, it was becoming hard to maintain.

This PR:

  • moves StripeObject into stripe.stripe_object

  • moves the abstract API classes (APIResource, CreateableAPIResource, UpdateableAPIResource, DeletableAPIResource, ListableAPIResource and VerifyMixin) into stripe.api_resources.abstract

  • moves all concrete API resource classes into stripe.api_resources

  • moves a bunch of utility methods (including convert_to_stripe_object) into stripe.util

  • aliases everything into stripe.resource to maintain backwards compatibility

Additionally, all concrete API classes now have an OBJECT_NAME class constant containing the name of the API resource. This is used by convert_to_stripe_object to map JSON to class instances. This brings the implementation much closer to what's done in stripe-ruby.

Tagging as WIP because there are still a few things I want to polish, but opening the PR now for feedback.

Copy link
Contributor

@brandur-stripe brandur-stripe left a comment

Choose a reason for hiding this comment

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

Incredible @ob-stripe!

It always really annoyed me that all the classes were in one file despite the tests being broken out separately! So you'd load up a test, try to find the equivalent file for it, but then realize you had to look in the giant resource.py.

Strongly in favor of the change in general. The only thing I noticed is that some of these import paths are getting a little long/unwieldy:

from stripe.api_resources.abstract.api_resource import APIResource

That's pretty subjective though, and in practice I don't think it'll matter very much.

url = self.instance_url() + '/verify'
headers = util.populate_headers(idempotency_key)
self.refresh_from(self.request('post', url, params, headers))
return self
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, this might be a little more abstraction than is necessary in that it saves some LOCs, but also makes the code more indirect and harder to follow. In practice the implementation ~never changes, so having it updatable in one place doesn't buy you all that much.

That said, given that it's already here, I think it'd be okay to leave it.

@ob-stripe
Copy link
Contributor Author

The only thing I noticed is that some of these import paths are getting a little long/unwieldy:

Yeah, I feel the same actually -- I'm torn between clear namespace separation and name brevity / clarity! It's one of the things I want to polish, I'll sleep on it and see if I can come up with something better tomorrow. If you have any suggestions, please do share.

@brandur-stripe
Copy link
Contributor

Yeah, I feel the same actually -- I'm torn between clear namespace separation and name brevity / clarity! It's one of the things I want to polish, I'll sleep on it and see if I can come up with something better tomorrow. If you have any suggestions, please do share.

I'm afraid that I haven't really thought of anything any better :)

I think the part that bothers me most is just that Python's module conventions unfortunately introduce some redundancy in cases like these in that really .api_resource and APIResource are the same thing, but you still need to address both of them.

I don't think it's going to be much of a problem in practice, so just let me know when I should bring this in.

@ob-stripe ob-stripe changed the title [WIP] Split resource.py Split resource.py Oct 20, 2017
@stripe-ci
Copy link

@ob-stripe
Copy link
Contributor Author

I pushed another commit to update all existing stripe.resource imports to use the new modules instead.

stripe.resource still exists and should still contain all the same references as before, so users who for some reason import directly from stripe.resource will not be affected by this change. We should get rid of stripe.resource in the next major version bump though.

@brandur-stripe
Copy link
Contributor

stripe.resource still exists and should still contain all the same references as before, so users who for some reason import directly from stripe.resource will not be affected by this change. We should get rid of stripe.resource in the next major version bump though.

Nice!

Alright, let's do this.

@brandur-stripe brandur-stripe merged commit d1962df into master Oct 20, 2017
@brandur-stripe brandur-stripe deleted the ob-split-resources branch October 20, 2017 17:00
@ob-stripe
Copy link
Contributor Author

\o/

@brandur-stripe
Copy link
Contributor

Released as part of 1.69.0!

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.

None yet

3 participants