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

refactor: used pydantic for object mapping #161

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lefcha
Copy link
Collaborator

@lefcha lefcha commented Feb 21, 2025

This is a rebase of the original PR #129 by @BoggerByte

This pull request replaces hardcoded object mapping methods with Pydantic models, enhancing flexibility and readability.

Changes Made

  • Consolidated object mapping methods from_dict and to_dict into a unified approach leveraging Pydantic's model capabilities.
  • Add some missing type annotations.
  • Addressed a bug related to models default values.

This PR resolves #128 and aligns the project with best practices for object mapping and data validation using Pydantic.
Your feedback and review are highly appreciated!

  • pytest passed
  • mypy no errors
  • ruff no errors

@lefcha lefcha self-assigned this Feb 21, 2025
@lefcha lefcha requested a review from proxi February 21, 2025 15:09
Copy link
Member

@proxi proxi left a comment

Choose a reason for hiding this comment

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

The direction of change looks good, but we cannot just add such a major dependency without bumping the major version. I asked @goncalossilva and he said:

revamping our Python SDK for vNEXT is on my list for the quarter. This will be done under a new major version, of course. And this way we can adopt Pydantic.

So I think we need to put this on hold for now.

@goncalossilva
Copy link
Member

Thank you for this, @lefcha & @BoggerByte!

Let's target a new branch, v3, and merge this there, please? @proxi do you have concers with that?

@BoggerByte
Copy link

BoggerByte commented Feb 21, 2025

I understand that choosing Pydantic as a dependency might have been a bit heavy in terms of size and actual usage scope. If all we need is to parse and serialize models, a better choice might be dataclasses-json. It’s essentially a lightweight alternative to Pydantic and, in my opinion, a perfect fit for this project 😃

Here's an example of dataclasses-json usage I found on their project main page:

@dataclass_json
@dataclass
class Person:
    name: str


person = Person(name='lidatong')
person.to_json()  # '{"name": "lidatong"}' <- this is a string
person.to_dict()  # {'name': 'lidatong'} <- this is a dict
Person.from_json('{"name": "lidatong"}')  # Person(1)
Person.from_dict({'name': 'lidatong'})  # Person(1)

@BoggerByte
Copy link

BoggerByte commented Feb 22, 2025

Switched to dataclasses_json for object mapping to simplify serialization and reduce overhead in new #163 PR. @proxi, @pkoch, @goncalossilva, let me know if you see any issues with this approach!

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.

Enhancement: Implement Object Mapping Using Pydantic / Msgspec
4 participants