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.

Sign up for GitHub

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

Jump to bottom

feat(internal): msgspec signature model and pydantic v2 support. #1854

Closed
wants to merge 7 commits into from
Closed

feat(internal): msgspec signature model and pydantic v2 support. #1854

wants to merge 7 commits into from

Conversation

peterschutt
Copy link
Contributor

@peterschutt peterschutt commented Jun 21, 2023 •
edited by Goldziher
Loading

This PR migrates to using msgspec as the basis for signature modelling. It removes the dependency on attrs and pydantic for this purpose and switching to using msgspec exclusively.

Goldziher
Goldziher reviewed Jun 21, 2023
View reviewed changes
Copy link
Contributor

@Goldziher Goldziher left a comment

Choose a reason for hiding this comment

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

Looks good.

The aim is to replace the other models with this, correct?

litestar/_signature/models/msgspec_signature_model.py Outdated Show resolved Hide resolved
@peterschutt
Copy link
Contributor Author

Looks good.

The aim is to replace the other models with this, correct?

Yes that's the plan! I'm just trying to get the tests passing with as few changes as possible, but ran out of time. Will pick up again tomorrow.

@peterschutt
Copy link
Contributor Author

The remaining failures are from exception handling/error formatting.

@sourcery-ai sourcery-ai bot mentioned this pull request Jun 27, 2023
Closed
@Goldziher
Copy link
Contributor

I rebased this on main and updated the implementation to align. I also squashed the previous commits together - so its easier to rebase.

I am going to branch from this now.

@Goldziher Goldziher force-pushed the msgspec-signature-model branch 3 times, most recently from 5f79d7e to 12e35f2 Compare July 7, 2023 19:51
@Goldziher Goldziher changed the title RFC: msgspec signature model. Feat: msgspec signature model. Jul 7, 2023
@Goldziher
Copy link
Contributor

I removed the pydantic and attrs logic and refactored the code, also fixed most tests. The only remaining issue for now is the error handling.

@Goldziher Goldziher changed the title Feat: msgspec signature model. feat(internal): msgspec signature model and pydantic v2 support. Jul 14, 2023
@Goldziher Goldziher marked this pull request as ready for review July 14, 2023 16:02
@Goldziher Goldziher requested a review from a team as a code owner July 14, 2023 16:02
@Goldziher
Copy link
Contributor

I merged the pydantic v2 support into this one. So it can be handled together.

@provinzkraut
Copy link
Member

provinzkraut commented Jul 14, 2023 •
edited
Loading

@Goldziher This is not ready for review at all. There are several things that need to be handled before this can go in.

@Goldziher
Copy link
Contributor

This is not ready for review at all. There are several things that need to be handled before this can go in.

such as?

@provinzkraut
Copy link
Member

The error handling will require a lot of substantial changes for example, and the support for Pydantic and attrs is only preliminary, since the intended solution (via plugins) hasn't been implemented yet.

I don't see why this has to be merged now, in an unfinished state, when we will then have to go back and alter is fundamentally again.

@Goldziher
Copy link
Contributor

Goldziher commented Jul 14, 2023 •
edited
Loading

The error handling will require a lot of substantial changes for example, and the support for Pydantic and attrs is only preliminary, since the intended solution (via plugins) hasn't been implemented yet.

I don't see why this has to be merged now, in an unfinished state, when we will then have to go back and alter is fundamentally again.

we do not need to alter it IMO. We can opt to alter it - but that can be done in iterations and is elective. In fact - IMO it should not go into this PR - whatever needs to come later, can come later and is not a blcoker here.

The attrs and pydantic stuff is fully functional noe. That is - it serializes, deserializes and generates schemas correctly.

Anyhow, i made a thread in discord.

@peterschutt @Goldziher
feat(internal): add msgspec signature model.
8f97076 
Linting fixes.

Update tests/unit/test_kwargs/test_path_params.py

fix signature namespace issue

support min_length and max_length

support min_length and max_length

handle constraints on union types

fix error message tests

chore(signature-model): remove pydantic and attrs signature models

chore(signature model): fix python 3.8 compat
@provinzkraut
Copy link
Member

we do not need to alter it

Yes we do, unless we don't care about breaking exceptions completely.

It is fully functional now already

It is not. As I said before, the handling of the exception will require substantial changes, and those are not ready at the moment.
We can argue about this all we want, but that doesn't change the facts.

@Goldziher
Copy link
Contributor

we do not need to alter it

Yes we do, unless we don't care about breaking exceptions completely.

It is fully functional now already

It is not. As I said before, the handling of the exception will require substantial changes, and those are not ready at the moment. We can argue about this all we want, but that doesn't change the facts.

the only thing that requires changes is the exception handling. But that is a given.

@Goldziher @peterschutt
Pydantic v2 integration (#1952)
6a7af34 
* feat(internal): add pydantic v2 support

Linting fixes.

Update tests/unit/test_kwargs/test_path_params.py

fix signature namespace issue

support min_length and max_length

support min_length and max_length

handle constraints on union types

fix error message tests

chore(signature-model): remove pydantic and attrs signature models

chore(signature model): fix python 3.8 compat

feat: msgspec signature model.

Linting fixes.

Update tests/unit/test_kwargs/test_path_params.py

fix signature namespace issue

support min_length and max_length

support min_length and max_length

handle constraints on union types

fix error message tests

chore(signature-model): remove pydantic and attrs signature models

feat: msgspec signature model.

Linting fixes.

Update tests/unit/test_kwargs/test_path_params.py

fix signature namespace issue

support min_length and max_length

support min_length and max_length

handle constraints on union types

fix error message tests

feat(signature-model): add pydantic v2 support

* feat(internal): handle pydantic errors

---------

Co-authored-by: Peter Schutt <peter.github@proton.me>
@Goldziher Goldziher force-pushed the msgspec-signature-model branch 3 times, most recently from 5931426 to 7354ab7 Compare July 15, 2023 09:02
@Goldziher Goldziher closed this Jul 15, 2023
@sonarcloud
Copy link

sonarcloud bot commented Jul 15, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

92.8% 92.8% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@github-actions
Copy link

Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/1854

@provinzkraut provinzkraut mentioned this pull request Aug 20, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JacobCoffee JacobCoffee JacobCoffee left review comments

@Goldziher Goldziher Goldziher left review comments

@provinzkraut provinzkraut provinzkraut left review comments

@jcrist jcrist jcrist left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@peterschutt @Goldziher @provinzkraut @jcrist @JacobCoffee