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

Towards pydantic dataclass #210

Merged
merged 5 commits into from
Mar 7, 2024

Conversation

mgcam
Copy link
Member

@mgcam mgcam commented Mar 5, 2024

No description provided.

Copy link
Collaborator

@nerdstrike nerdstrike left a comment

Choose a reason for hiding this comment

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

I see nothing bad here. I think you've found the best way yet.

Copy link
Collaborator

@nerdstrike nerdstrike left a comment

Choose a reason for hiding this comment

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

One minor doc error to correct.

I like the approach. We could at some point extend pydantic.base_model to include the get_field_names method so it is usable everywhere. pre_root is almost general, but still knows about the sqlalchemy table it maps to. Do you think we can meaningfully have a table-mapping pre_root that can do the job for any sqlalchemy-single-table -> class transformation?

of the well.

The best way to instantiate the object of this class is via the constructor,
giving it the an ORM object representing a database row with information
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor grammar confusion.

@mgcam
Copy link
Member Author

mgcam commented Mar 7, 2024

Do you think we can meaningfully have a table-mapping pre_root that can do the job for any sqlalchemy-single-table -> class transformation?

Yes, to some degree. I expect we will factor out some functionality into stand-alone functions as we convert more classes ot this type of model.

@mgcam
Copy link
Member Author

mgcam commented Mar 7, 2024

We could at some point extend pydantic.base_model to include the get_field_names

I am not sure whether the two types of models we use have a common ancestor. If we need this functionality, we should certainly thing about pushing it up to an ancestor or to a stand-alone utility package

@mgcam mgcam force-pushed the towards_pydantic_dataclass branch from 9bf555b to 4d2efbd Compare March 7, 2024 16:06
@nerdstrike nerdstrike merged commit 732f1ba into wtsi-npg:devel Mar 7, 2024
4 checks passed
@mgcam mgcam deleted the towards_pydantic_dataclass branch June 7, 2024 17:11
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.

2 participants