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

[Suggestion] use dataclass or pydantic instead of attr for models. #10

Open
ahnsv opened this issue Jul 5, 2023 · 5 comments · May be fixed by #35
Open

[Suggestion] use dataclass or pydantic instead of attr for models. #10

ahnsv opened this issue Jul 5, 2023 · 5 comments · May be fixed by #35
Labels
enhancement New feature or request

Comments

@ahnsv
Copy link

ahnsv commented Jul 5, 2023

Hi, just bumped into this repo while I was browsing apple's changed receipt validation policy.
Though it seems like this repo is its early stage, I think it would be more straightforward for potential users to use dataclass or pydantic.

@ahnsv
Copy link
Author

ahnsv commented Jul 5, 2023

PS: I just found this thread, but still the question is unanswered. If it is okay, want to change the models into dataclass and stuffs.

@alexanderjordanbaker
Copy link
Collaborator

Could you please elaborate why it would be more straightforward for potential users?

@ahnsv
Copy link
Author

ahnsv commented Jul 12, 2023

Sorry for the late reply.
The reason why I thought it would be better to use dataclass or pydantic instead of attrs is :

  1. simplicity - attrs needs users to install additional dependency, while dataclass is in native python since 3.7
  2. readability - IMHO attrs method name is not explicit enough for users (e.g., attrs.ib, attrs.s, and stuff...), while dataclasses fields are more explicit and readable.
  3. popularity - dataclasses module has been widely adopted in many projects (attrs would be as well), and pydantic has gained more popularity with the rise of FastAPI. Going with dataclass or pydantic will give more chances to contribute to modern python users.
  4. functionality (maybe overkill in this usecase?) - as far as I know, attrs would be a good choice when you need validation in data serialization with validation but it seems like no validation applied in the repo (just default = None). I don't know if it will be added in the future, but for now I think dataclass would be enough to do the work.

And here are some references I looked up:
https://www.revsys.com/tidbits/dataclasses-and-attrs-when-and-why/
https://site-deploy--pydantic-docs.netlify.app/benchmarks/
https://threeofwands.com/why-i-use-attrs-instead-of-pydantic/ - This article says attrs over pydantic but pydantic's latest release (v2) is significantly better than the previous, so you can check it out too.

@alexanderjordanbaker alexanderjordanbaker added the enhancement New feature or request label Aug 7, 2023
@elonzh
Copy link
Contributor

elonzh commented Sep 2, 2023

In addition to those reasons listed above, pydantic is used in many web framework like fastapi. We can combines models with other server parts if using pydantic.

For example:

  • appstoreserverlibrary.models.ResponseBodyV2.ResponseBodyV2 defines the server notification model, but we can't use it as a fastapi request body when it is not a pydantic model.
  • Normally we will save the app store transaction info into database and expose app server transaction with those data in admin api. We can combine models like JWSTransactionDecodedPayload if it's a pydantic model.

@ahnsv ahnsv linked a pull request Sep 17, 2023 that will close this issue
@elonzh
Copy link
Contributor

elonzh commented Nov 7, 2023

More and more projects are beginning to use pydantic. For example OpenAI just released their new SDK(v1.0) with httpx and pydantic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants