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

Drop the use of sea-strum and depends on the original strum with a tailored EnumIter provided #1535

Merged
merged 9 commits into from
Mar 22, 2023

Conversation

billy1624
Copy link
Member

@billy1624 billy1624 commented Mar 11, 2023

PR Info

New Features

Upgrades

  • Upgrade strum to 0.24

Breaking Changes

  • Added derive and strum features to sea-orm-macros
  • Replaced sea-strum dependency with strum in sea-orm
  • Re-exported sea_orm_macros::EnumIter instead of strum::EnumIter on the root of sea-orm

@billy1624 billy1624 requested a review from tyt2y3 March 11, 2023 09:18
@billy1624 billy1624 self-assigned this Mar 11, 2023
@billy1624 billy1624 modified the milestone: 0.12.x Mar 11, 2023
@billy1624 billy1624 mentioned this pull request Mar 11, 2023
1 task
@billy1624 billy1624 marked this pull request as ready for review March 11, 2023 09:44
@@ -29,4 +29,7 @@ sea-orm = { path = "../", features = ["macros"] }
serde = { version = "1.0", features = ["derive"] }

[features]
default = ["derive"]
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need a feature here? I think we always want to enable this right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Backward compatibility. Not a must, I just think it's better to ship with derive by default like it used to be.

Copy link
Member Author

Choose a reason for hiding this comment

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

We have two features in sea-orm-macros

  1. derive - provide all derive macros of SeaORM, e.g. DeriveEntityModel
  2. strum - provide the EnumIter derive macros

I split the sea-orm-macros in half because one can opt-out the derive feature in SeaORM by specifying sea-orm = { version = "*", default-features = false }. Where the sea-orm-macros is a optional dependency. But now, the strum reside in it, making it a "compulsory" dependency. Instead, we turn on / off the derive feature as needed.

In contrast, we ALWAYS want the strum feature to be enabled.

Hence,

- sea-orm-macros = { version = "0.11.0", path = "sea-orm-macros", default-features = false, optional = true }
+ sea-orm-macros = { version = "0.11.0", path = "sea-orm-macros", default-features = false, features = ["strum"] }

[features]
- macros = ["sea-orm-macros", "sea-query/derive"]
+ macros = ["sea-orm-macros/derive", "sea-query/derive"]

Copy link
Member

Choose a reason for hiding this comment

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

Umm. This sounds complicated. Well but let it be for now.

postgres-array = []
derive = ["bae"]
strum = []
Copy link
Member

@tyt2y3 tyt2y3 Mar 21, 2023

Choose a reason for hiding this comment

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

Same, I think we can just ship it by default

Copy link
Member Author

Choose a reason for hiding this comment

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

For this, dependency are conditionally enabled when a feature is being enabled.

This should be fine. If we decided to keep derive and strum as it's now.

@billy1624 billy1624 changed the base branch from update-heck-dependency to master March 21, 2023 09:01
@billy1624 billy1624 requested a review from tyt2y3 March 21, 2023 09:06
Copy link
Member

@tyt2y3 tyt2y3 left a comment

Choose a reason for hiding this comment

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

Awesome

@billy1624 billy1624 merged commit 2eda8aa into master Mar 22, 2023
@billy1624 billy1624 deleted the depends-on-strum-directly branch March 22, 2023 03:47
darkmmon added a commit to darkmmon/seaql.github.io that referenced this pull request Jul 12, 2023
billy1624 added a commit to SeaQL/seaql.github.io that referenced this pull request Jul 19, 2023
* Optional Field SeaQL/sea-orm#1513

* .gitignore SeaQL/sea-orm#1334

* migration table custom name SeaQL/sea-orm#1511

* OR condition relation SeaQL/sea-orm#1433

* DerivePartialModel SeaQL/sea-orm#1597

* space for migration file naming SeaQL/sea-orm#1570

* composite key up to 12 SeaQL/sea-orm#1508

* seaography integration SeaQL/sea-orm#1599

* QuerySelect SimpleExpr SeaQL/sea-orm#1702

* sqlErr SeaQL/sea-orm#1707

* migration check SeaQL/sea-orm#1519

* postgres array SeaQL/sea-orm#1565

* param intoString SeaQL/sea-orm#1439

* **skipped** re-export SeaQL/sea-orm#1661

* ping SeaQL/sea-orm#1627

* on empty do nothing SeaQL/sea-orm#1708

* on conflict do nothing SeaQL/sea-orm#1712

* **skipped** upgrade versions

* active enum fail safe SeaQL/sea-orm#1374

* relation generation check SeaQL/sea-orm#1435

* entity generation bug SeaQL/sea-schema#105

* **skipped** bug fix that does not require edits

* EnumIter change SeaQL/sea-orm#1535

* completed and fixed a previous todo SeaQL/sea-orm#1570

* amended wordings and structures

* Edit

* Remove temp file

---------

Co-authored-by: Billy Chan <ccw.billy.123@gmail.com>
@github-actions
Copy link

github-actions bot commented Aug 2, 2023

🎉 Released In 0.12.1 🎉

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
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants