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

Dump SeaORM Dependency from SeaSchema's Migrator #59

Closed
wants to merge 3 commits into from

Conversation

billy1624
Copy link
Member

@billy1624 billy1624 commented Apr 8, 2022

PR Info

Adds

Multiple traits, structs and enums are added to attach with SeaORM

  • E.g. ConnectionTrait, QueryResultTrait, Statement, DatabaseBackend, MigrationErr

Breaking Changes

  • Migrator and CLI are moved to SeaORM leaving only the SchemaManager behind
  • Migration prelude module are moved to SeaORM as well

@tyt2y3
Copy link
Member

tyt2y3 commented Apr 9, 2022

I think it might be easier to just remove the migrator from sea-schema altogether, keeping only the SchemaManager.
And it is actually possible to just share the SQLx connections directly between the two crates, that way we don't have to split out SeaORM's connections.

@billy1624
Copy link
Member Author

That would be better. This way we can avoid introducing additional connection related traits on sea-schema side

@billy1624
Copy link
Member Author

I think it might be easier to just remove the migrator from sea-schema altogether, keeping only the SchemaManager.

But then we still need a way to get the connection out of SchemaManager for user to seed the db with SeaORM for instance. And, this is why I have...

impl<'c, C> SchemaManager<'c, C>
where
    C: MigrationConnection,
{
    pub fn get_connection(&self) -> &'c C::Connection {
        self.conn.get_connection()
    }
}

And it is actually possible to just share the SQLx connections directly between the two crates, that way we don't have to split out SeaORM's connections.

If we only keep SchemaManager we don't need a SQLx connection in sea-schema. Btw... if we only keep SchemaManager, why don't we move all the migration things into SeaORM? SchemaManager is just a wrapper of SeaQuery statements and some db utilities (column / table exists).

@tyt2y3
Copy link
Member

tyt2y3 commented Apr 11, 2022

But then we still need a way to get the connection out of SchemaManager for user to seed the db with SeaORM for instance

It should work the other way around, downstream (sea-orm) lend the connection to SeaSchema instead of SeaSchema owning it

@tyt2y3
Copy link
Member

tyt2y3 commented Apr 11, 2022

I think it's worthwhile to have clear-cut crate boundary between things

@billy1624
Copy link
Member Author

It should work the other way around, downstream (sea-orm) lend the connection to SeaSchema instead of SeaSchema owning it

SeaORM lend the inner SQLx connection to SeaSchema?

@tyt2y3
Copy link
Member

tyt2y3 commented Apr 13, 2022

Yes

@tyt2y3
Copy link
Member

tyt2y3 commented Apr 18, 2022

Hopefully we can untangle this in 0.8.0

@billy1624 billy1624 force-pushed the dump-sea-orm-dep branch 3 times, most recently from e52651b to 082ca82 Compare April 20, 2022 05:12
@billy1624 billy1624 marked this pull request as ready for review April 20, 2022 06:40
@billy1624
Copy link
Member Author

Hopefully we can untangle this in 0.8.0

That's the target! Please review this PR @tyt2y3

use sea_orm::{Database, DbConn};
use std::{fmt::Display, process::exit};
use tracing_subscriber::{prelude::*, EnvFilter};
//! Migrator CLI utility
Copy link
Member

@tyt2y3 tyt2y3 Apr 30, 2022

Choose a reason for hiding this comment

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

we should drop this file and clap dependency entirely

Copy link
Member Author

Choose a reason for hiding this comment

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

The clap subcommand was used on sea-orm-cli and sea-orm-migration, both of them shared the save subcommand that's why I put it in sea-schema.

I have removed clap dependency and rewrite it as macros, 644741a.

Copy link
Member Author

Choose a reason for hiding this comment

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

See if that looks good @tyt2y3

@tyt2y3
Copy link
Member

tyt2y3 commented Apr 30, 2022

A library crate cannot have clap as dependency

@tyt2y3
Copy link
Member

tyt2y3 commented Apr 30, 2022

Otherwise it looks good on sea-schema side

@tyt2y3 tyt2y3 closed this May 8, 2022
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.

Dump SeaORM Dependency from SeaSchema's Migrator
2 participants