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

Rework ActiveEnum to support TryGetable for Vec<Json> #1898

Merged
merged 4 commits into from Oct 16, 2023

Conversation

tyt2y3
Copy link
Member

@tyt2y3 tyt2y3 commented Oct 4, 2023

Succeeds #1834

Step 1: remove the reliance on ValueVec

@tyt2y3 tyt2y3 force-pushed the rework-active-enum branch 3 times, most recently from 1406115 to b2cb513 Compare October 4, 2023 22:39
@tyt2y3 tyt2y3 changed the title Rework ActiveEnum Rework ActiveEnum to support TryGetable for Vec<Json> Oct 4, 2023
@tyt2y3
Copy link
Member Author

tyt2y3 commented Oct 4, 2023

@anshap1719 I think I figured out something within the type system. At least it compiles, so we won't be too far away from complete. I can't really explain my course of thinking, but it wandered a bit and ah-ha!

If you have time, can you help verify and perhaps add back some of the test cases you made?

@anshap1719
Copy link
Sponsor Contributor

@tyt2y3 Perfect! Thank you. My first approach was to try to get rid of ValueVec, but I didn't persue it because of lack of a holistic picture of the project.

In any case, I'll re-add the test files and fixtures this weekend if you haven't added them already by then.

Copy link
Sponsor Contributor

@anshap1719 anshap1719 left a comment

Choose a reason for hiding this comment

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

@tyt2y3 I'm not sure how I feel about having to manually implement NotU8 for types where I need to use them as root JSON arrays. It feels like that's an internal implementation detail that shouldn't be exposed to users of the crate.

Not sure what the solution would be either, in my mind the straightforward one would be to have a derive macro that either just implements NotU8 for target, or something like FromJsonQueryArrayResult that internally just derives FromJsonQueryResult as well as NotU8.

sea-orm-macros/src/derives/active_enum.rs Outdated Show resolved Hide resolved
@anshap1719
Copy link
Sponsor Contributor

Oh by the way, apart from things that I mentioned above, this does solve the original issue. I'm able to use root JSON arrays just fine with this branch.

@tyt2y3
Copy link
Member Author

tyt2y3 commented Oct 8, 2023

How cool is that! Let's get this done this few days.

@anshap1719
Copy link
Sponsor Contributor

@tyt2y3 Created #1900 for adding tests and fixing issue I pointed above.

* Add Tests For Root JSON Arrays And Active Enum Vectors

* Fix Build Issue When Using `DeriveActiveEnum`
@tyt2y3
Copy link
Member Author

tyt2y3 commented Oct 12, 2023

I figured we could make this work even on SQLite, provided we enable the postgres-array feature flag. Not ideal, but Good enough for now.

@anshap1719
Copy link
Sponsor Contributor

Hi @tyt2y3 Sorry for not being able to get back in time. The changes you had to make in column.rs was precisely why it was taking me time because insert tests weren't working for me locally.

Thank you for merging the PR and continuing on with necessary changes.

@tyt2y3
Copy link
Member Author

tyt2y3 commented Oct 12, 2023

Is it going to pass this time? I am thrilled

@tyt2y3
Copy link
Member Author

tyt2y3 commented Oct 12, 2023

image
(image of high-five)

@IgnisDa
Copy link
Contributor

IgnisDa commented Oct 12, 2023

Hey @tyt2y3 @anshap1719! Will this PR fix #1517?

@anshap1719
Copy link
Sponsor Contributor

@IgnisDa Yes. I have already confirmed locally that it solves #1517

@tyt2y3 tyt2y3 linked an issue Oct 12, 2023 that may be closed by this pull request
@IgnisDa
Copy link
Contributor

IgnisDa commented Oct 12, 2023

Awesome! Any ETA as to when it would be merged?

@tyt2y3 tyt2y3 merged commit 77ae46f into master Oct 16, 2023
26 checks passed
@tyt2y3 tyt2y3 deleted the rework-active-enum branch October 16, 2023 09:05
@anshap1719
Copy link
Sponsor Contributor

Thanks for all the efforts @tyt2y3

@IgnisDa
Copy link
Contributor

IgnisDa commented Oct 16, 2023

@anshap1719 What underlying data type are you using for this column?

@tyt2y3
Copy link
Member Author

tyt2y3 commented Oct 16, 2023

Thank you everyone. I am happy for what we've achieved out of this collaboration!

@anshap1719
Copy link
Sponsor Contributor

@tyt2y3 Do you know when we can expect the next release? Not trying to push for it, just want to understand the timeline. Thanks.

@tyt2y3
Copy link
Member Author

tyt2y3 commented Oct 19, 2023

I am trying to do it this week :)

@github-actions
Copy link

🎉 Released In 0.12.4 🎉

Thank you everyone for the contribution!
This feature is now available in the latest release. Now is a good time to upgrade!
Your participation is what makes us unique; your adoption is what drives us forward.
You can support SeaQL 🌊 by starring our repos, sharing our libraries and becoming a sponsor ⭐.

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.

Add support for root arrays in JSON in SeaORM
3 participants