Skip to content

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
Contributor

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
Contributor

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!

@goncalossilva
Copy link
Member

This is great, @BoggerByte, thanks!

I'll pick this up over the next couple of weeks as I work on a version bump for this library. 🙌

@goncalossilva
Copy link
Member

Replaced by #163.

@goncalossilva goncalossilva deleted the BoggerByte-feature/pydantic-object-mapping branch March 30, 2025 22:20
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

5 participants