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

Rename API resource's request method #936

Merged
merged 1 commit into from
Aug 5, 2020

Conversation

brandur
Copy link
Contributor

@brandur brandur commented Aug 4, 2020

As seen in #928, the refresh method doesn't work for an event class.
This is because event has a field called request, and it ends up
replacing the request method that it inherited from being an API
resource, so when refresh tries to make a request, it fails because it
tries to invoke it on the accessor added for the event's property.

Here we give request a much more unique name so that it will never
conflict with a property field again, and update all internal references
to use the new name. We use alias to make the old name available for
backwards compatibility reasons because its been around for so long that
people are probably calling it.

Fixes #928.

r? @ob-stripe @richardm-stripe

@brandur-stripe
Copy link
Contributor

The other thing I considered while writing this was renaming request_stripe_object to be more consistent, or try to standardize its parameters with execute_resource_request (they take the same parameters, but they're named and optional in the former and positional in the latter), but ... overall it didn't seem 100% worth it.

I do think though that in general we need to clean up a lot of the naming and modules in this library. Like, it really doesn't make too much practical sense for some request-related methods to be in the Request module and others to be in APIResource. I don't think we actually need this level of composability anywhere in practice, and it's all become pretty tightly intertwined anyway, so we should probably just flatten everything out to make it less confusing.

@brandur-stripe
Copy link
Contributor

(I also have a small internal PR to reflect these changes that I'll assign to whomever ends up looking at this one.)

Copy link
Contributor

@richardm-stripe richardm-stripe left a comment

Choose a reason for hiding this comment

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

This looks good to me, happy to look at the internal PR too.

@brandur-stripe
Copy link
Contributor

Thanks a lot Richard! Just assigned you the internal version as well.

As seen in #928, the `refresh` method doesn't work for an event class.
This is because event has a field called `request`, and it ends up
replacing the `request` method that it inherited from being an API
resource, so when `refresh` tries to make a request, it fails because it
tries to invoke it on the accessor added for the event's property.

Here we give `request` a much more unique name so that it will never
conflict with a property field again, and update all internal references
to use the new name. We use `alias` to make the old name available for
backwards compatibility reasons because its been around for so long that
people are probably calling it.

Fixes #928.
@brandur-stripe brandur-stripe merged commit 3433130 into master Aug 5, 2020
@brandur-stripe brandur-stripe deleted the brandur-rename-request branch August 5, 2020 23:00
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.

Event#refresh doesn't work
4 participants