-
Notifications
You must be signed in to change notification settings - Fork 445
RFC: What if StripeObject
didn't inherit from dict
?
#1454
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
Comments
I think not extending dict is the way to go. Maybe you should support both behaviors by having a DictClient returning the current behavior and another one maybe using Pydantic models instead. This would help a lot with autocomplete and data validation via Static analyse but also at runtime before it even calls the API. This would also probably help to build a mock client for unit testing |
I'm in favour of @Benoss suggestion of having Pydantic models Another issue that it would solve is that some fields marked as Optional actually might not even be available at all. e.g. class Customer:
...
deleted: Optional[Literal[True]]
"""
Always true for a deleted object
""" But I use File "stripe_hook.py", line 91, in _handle_subscription_upserted
await self._update_database_subscription(subscription.id, subscription)
File "stripe_hook.py", line 194, in _update_database_subscription
if customer.deleted:
File "$HOME/.local/share/virtualenvs/core-bRkH1-KB/lib/python3.11/site-packages/stripe/_stripe_object.py", line 172, in __getattr__
raise AttributeError(*err.args) from err
AttributeError: deleted Failures like this decrease my confidence that my code will just work. I do not know which other fields might go wrong. I strongly support a single breaking change to migrate to using Pydantic models. I expect this would resolve all these issues in a single swipe. A lot of behaviour is built in to pydantic that supports the use-cases you mention:
Can use
I wouldn't expect this to work, but Pydantic can do something better. Dump a dict, make changes, then validate that they are correct: customer_dict = customer.model_dump()
customer_dict["deleted"] = True
new_customer = Customer.model_validate(**customer_dict) |
Background
The objects you get back from the Stripe API all share a common base class:
StripeObject
(source). It holds the raw API response and has helpers for reading, writing, and updating data using a convenient dot notation:obj.name.whatever
.For many years,
StripeObject
has inherited fromdict
, which means you getdict
-like behavior "for free":len(obj)
shows how many keys are in theStripeObject
,for key in obj
iterates through those keys, etc.While implementing all of the
dict
methods confers certain advantages when working with maps of data, there are also drawbacks. Most notably, any API response fields that share a name with a built-in method are inaccessible with dot notation:Over the years, we've gotten many issues filed about this counterintuitive behavior. But, we're hesitant to make breaking changes for behavior that's existed for so long. To better understand the implications of these changes, we're soliciting developer feedback to make sure we're serving your needs well.
Proposed Solution
Our proposal is that
StripeObject
would not inherit from anything. As a result, it would lose its dict-like behavior (len(obj)
,obj.items()
,{**obj}
,json.dumps(obj)
etc) but retain property access via dot notation. Most documented examples don't take advantage (or even mention) thatStripeObject
s are dictionaries, so we're hoping to not lose much functionality here.In the interest of minimizing the impact of this change, we'd add a
StripeObject.as_dict()
(name not final) method to return the underlying dictionary and enable all the behavior from before. As a result, the migration to keep existing behavior would be clear and relatively simple: call that method when you needdict
-specific functionality.Impact
As a result of no longer being a
dict
, any. This includes (but is not limited to):StripeObject
to a function usingsome_func(**stripe_obj)
StripeObject
to json:json.dumps(stripe_obj)
would need thedefault=vars
kwargisintance(obj, dict)
checks would change behavior, if you have any of thoseAlternative Solution
We have an existing mechanism to separate the name of the class property from the API value. We already use this for
tax.Registration.CountryOptions.Is
(docs) becausetax.registration.country_options.is
is aSyntaxError
; we useis_
instead.We could do the same thing for
subscription.items
..items
would continue to bedict.items()
and.items_
would be the list ofSubscriptionItem
s. The type annotations would indicate this and the existingobj['items']
approach would continue to work.We're not thrilled with this because it doesn't match the data you get from the API and it's an extra thing to think about and remember. But, in a world where most users write code with rich typing support, it may not be a big deal. Plus, this wouldn't be a breaking change, which is useful for reducing general churn and toil.
Our Questions for You:
some_obj.as_dict()
and made modifications to that dictionary, would you expect those changes to be reflected insome_obj
?. Does your answer change if the method is calledto_dict()
instead?items
bug using the alternative solution mentioned above and kept the currentdict
inheritance? Why or why not?Feel free to provide any other suggestions, comments, or use cases that you think can be helpful. And as always, thank you for helping us make the Python SDK the best it can be!
See Also
items
-attribute on Subscription object #297The text was updated successfully, but these errors were encountered: