-
Notifications
You must be signed in to change notification settings - Fork 40
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
feat: add support for SQLAlchemy dataclass-mapped tables #518
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #518 +/- ##
==========================================
- Coverage 91.42% 91.34% -0.08%
==========================================
Files 12 13 +1
Lines 1901 1919 +18
Branches 401 402 +1
==========================================
+ Hits 1738 1753 +15
- Misses 107 110 +3
Partials 56 56 ☔ View full report in Codecov by Sentry. |
I added only some simple tests at the moment, but reaching codecov target is unfeasible without bloat testing everything after |
124ec10
to
31d457f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @barsa-net
Thank you for contributing to pyserde! 🙏
Please do the followings and the PR can be merged.
- Run test without sqlalchemy (See the comments in test_sqlalchemy.py)
- Add simple example (See 👥 Add @JWSong as a contributor #522)
- Add documentation in https://github.com/yukinarit/pyserde/blob/main/docs/en/types.md
fullname: Mapped[str] = mapped_column(String(30)) | ||
|
||
user = User(1, "john", "John Doe") | ||
assert user == de(User, se(user)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you run tests without sqlalchemy?
You can manually uninstall sqlalchemy and run tests here IMO
https://github.com/yukinarit/pyserde/blob/main/.github/workflows/test.yml#L32-L38
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure can do, but I have a unrelated doubt regarding https://github.com/yukinarit/pyserde/blob/main/.github/workflows/test.yml#L34
All tests are being run through poetry run ...
while orjson is being installed (seemingly) outside the virtual enviroment with pip install orjson
, it should be poetry install --extras orjson
or do I miss something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. This line had been added two years ago by this commit but apparently pyserde was never tested with orjson 🤣
217ce89
to
99ad356
Compare
Just to make it clear and avoiding misunderstanding about the scope of this PR, SQLAlchemy is a HUGE project with countless ways to declare tables, at the moment I plan to add only "basic" support to declarative dataclass mapping. As such, I suggest it should be annotated as an "experimental" feature, as some features at the moment don't work, e.g.:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@barsa-net is it ready for merge, or you would be adding some more changes? |
Sorry, I have been busy those days. |
Sorry for the delay, I added an additional test for nested SQLAlchemy models and I made clear in the documentation that the feature has to be considered experimental and as such some features could not work. I consider the PR "complete", if I manage to get some other feature working together with SQLAlchemy I'll eventually open another PR. |
Resolves #517
This hopefully add support for SQLAlchemy dataclass-mapped tables without breaking anything else
The following code should run without errors: