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

Create Table Statement #102

Merged
merged 12 commits into from
Sep 3, 2021
Merged

Create Table Statement #102

merged 12 commits into from
Sep 3, 2021

Conversation

bobbyng626
Copy link
Contributor

#59

@billy1624
Copy link
Member

Hey @bobbyng626, can you add some test cases? I saw you got some for debugging during development.

E.g. asserting the output of create_table_statement_of against a handwritten CreateTableStatement

@tyt2y3 tyt2y3 requested a review from billy1624 August 28, 2021 07:36
src/entity/create_stmt.rs Outdated Show resolved Hide resolved
src/entity/create_stmt.rs Outdated Show resolved Hide resolved
src/entity/create_stmt.rs Outdated Show resolved Hide resolved
src/entity/create_stmt.rs Outdated Show resolved Hide resolved
src/entity/mod.rs Outdated Show resolved Hide resolved
@tyt2y3 tyt2y3 marked this pull request as draft August 28, 2021 10:47
@tyt2y3 tyt2y3 force-pushed the master branch 2 times, most recently from 20580c3 to 8ce4b0a Compare August 29, 2021 19:33
@tyt2y3 tyt2y3 linked an issue Aug 30, 2021 that may be closed by this pull request
@billy1624 billy1624 self-assigned this Aug 31, 2021
@tyt2y3 tyt2y3 force-pushed the master branch 3 times, most recently from 2d2fff7 to 6847dab Compare August 31, 2021 08:02
@billy1624 billy1624 marked this pull request as ready for review August 31, 2021 15:29
@tyt2y3
Copy link
Member

tyt2y3 commented Aug 31, 2021

Thinking should we include on_update & on_delete settings on sea_orm::Relation

Seemingly yes

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 31, 2021

Thinking should we include on_update & on_delete settings on sea_orm::Relation

Seemingly yes

We don't have to change the codegen for now.

I think it's worth to add because then we don't have to modify the original test cases.

@billy1624
Copy link
Member

billy1624 commented Sep 1, 2021

Btw... why we have a seperate sea_orm::ColumnType that partially mirroring sea_query::ColumnType?
https://github.com/bobbyng626/sea-orm/blob/1ad1767457a5e320692b9a61db7d60560695c11c/src/entity/column.rs#L13-L36

I guess you don't want to expose sea_query? i.e. requiring user to supply one of sea_orm::ColumnType. So, maybe we can type def sea_query::ColumnType in sea-orm?

Like I did for sea_query::ForeignKeyAction at 5073c6f

pub type ForeignKeyAction = sea_query::ForeignKeyAction;

@tyt2y3
Copy link
Member

tyt2y3 commented Sep 1, 2021

Two reasons:

  1. I want it to have the def method. It shouldn't be a problem though. We can make a custom trait indeed.

  2. I think for example some numeric columns, the precision parameter is completely irrelevant and is being deprecated anyway.

So either we break SeaQuery, or we have to do the extra mapping in SeaORM.

Cargo.toml Outdated
@@ -32,7 +32,7 @@ futures = { version = "^0.3" }
futures-util = { version = "^0.3" }
rust_decimal = { version = "^1", optional = true }
sea-orm-macros = { version = "^0.1.1", optional = true }
sea-query = { version = "^0.15", features = ["thread-safe"] }
sea-query = { version = "^0.15", git = "https://github.com/SeaQL/sea-query.git", features = ["thread-safe"] }
Copy link
Member

Choose a reason for hiding this comment

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

Use sea-query on crates.io after new version published

Copy link
Member

Choose a reason for hiding this comment

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

Yea 0.16 is coming

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

image

@tyt2y3 tyt2y3 merged commit ca13b1b into SeaQL:master Sep 3, 2021
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.

Transforming an Entity into TableCreateStatement
3 participants