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

Bump clap version to 3.2 #706

Merged
merged 4 commits into from
Jun 26, 2022
Merged

Bump clap version to 3.2 #706

merged 4 commits into from
Jun 26, 2022

Conversation

smonv
Copy link
Contributor

@smonv smonv commented May 10, 2022

PR Info

Changes

  • sea-orm-cli: bump clap version to 3.1.17
  • sea-orm-migration: bump clap version to 3.1.17

@ikrivosheev
Copy link
Member

I think it is better to rewrite with the derive clap feature.

@smonv
Copy link
Contributor Author

smonv commented May 10, 2022

I think it is better to rewrite with the derive clap feature.

Currently I don't have much experience with clap derive so this MR just try to bump and resolve all breaking changes, keeping current behavior.

If the decision still is rewrite to derive, I hope we can continue as another issue I'll try to working on it.

@ikrivosheev
Copy link
Member

I think it is better to rewrite with the derive clap feature.

Currently I don't have much experience with clap derive so this MR just try to bump and resolve all breaking changes, keeping current behavior.

If the decision still is rewrite to derive, I hope we can continue as another issue I'll try to working on it.

You can push changes with rewrite to derive in this branch. Code is built more maintainable with derive clap feature.
Can i help you with something?

@smonv
Copy link
Contributor Author

smonv commented May 10, 2022

I think it is better to rewrite with the derive clap feature.

Currently I don't have much experience with clap derive so this MR just try to bump and resolve all breaking changes, keeping current behavior.
If the decision still is rewrite to derive, I hope we can continue as another issue I'll try to working on it.

You can push changes with rewrite to derive in this branch. Code is built more maintainable with derive clap feature. Can i help you with something?

I'll try update to use clap derive. Currently I'm not sure what to ask, so I will ping you if necessary but I think it will be around meaning of some flag or env variables because I'm not familiar with functionality of all flags.

@igotfr
Copy link

igotfr commented May 10, 2022

the changes in the files main.rs and sea.rs are overengineering

@igotfr
Copy link

igotfr commented May 10, 2022

in cli.rs, lines 18 .env("DATABASE_URL"), and 29 .env("DATABASE_SCHEMA"),:

no method named `env` found for struct `Arg` in the current scope
method not found in `Arg<'_>`rustc[E0599](https://doc.rust-lang.org/error-index.html#E0599)

You probably don't see that because clap version 2.34 is necessary for sea-schema

@igotfr
Copy link

igotfr commented May 10, 2022

in cli.rs, lines 110 until 113:

for subcommand in get_subcommands() {
    migrate_subcommands =
        migrate_subcommands.subcommand(subcommand.arg(arg_migration_dir.clone()));
}
PS C:\Users\Usuario\prust\sea-orm2\sea-orm-cli> cargo check
    Checking sea-orm-cli v0.7.2 (C:\Users\Usuario\prust\sea-orm2\sea-orm-cli)
error[E0277]: the trait bound `clap::args::arg::Arg<'_, '_>: From<Arg<'_>>` is not satisfied
   --> src\cli.rs:110:59
    |
110 |             migrate_subcommands.subcommand(subcommand.arg(arg_migration_dir.clone()));
    |                                                       --- ^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `From<Arg<'_>>` is not implemented for `clap::args::arg::Arg<'_, '_>`
    |                                                       |
    |                                                       required by a bound introduced by this call
    |
    = help: the following implementations were found:
              <clap::args::arg::Arg<'a, 'b> as From<&'z clap::args::arg::Arg<'a, 'b>>>
    = note: required because of the requirements on the impl of `Into<clap::args::arg::Arg<'_, '_>>`
for `Arg<'_>`
note: required by a bound in `clap::app::App::<'a, 'b>::arg`
   --> C:\Users\Usuario\.cargo\registry\src\github.com-1ecc6299db9ec823\clap-2.34.0\src\app\mod.rs:796:19
    |
796 |     pub fn arg<A: Into<Arg<'a, 'b>>>(mut self, a: A) -> Self {
    |                   ^^^^^^^^^^^^^^^^^ required by this bound in `clap::app::App::<'a, 'b>::arg`

error[E0277]: the trait bound `App<'_>: From<clap::app::App<'_, '_>>` is not satisfied
   --> src\cli.rs:105:44
    |
110 |             migrate_subcommands.subcommand(subcommand.arg(arg_migration_dir.clone()));
    |                                 ---------- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait 
`From<clap::app::App<'_, '_>>` is not implemented for `App<'_>`
    |                                 |
    |                                 required by a bound introduced by this call
    |
    = note: required because of the requirements on the impl of `Into<App<'_>>` for `clap::app::App<'_, '_>`
note: required by a bound in `App::<'help>::subcommand`
   --> C:\Users\Usuario\.cargo\registry\src\github.com-1ecc6299db9ec823\clap-3.1.12\src\build\command.rs:367:26
    |
367 |     pub fn subcommand<S: Into<App<'help>>>(mut self, subcmd: S) -> Self {
    |                          ^^^^^^^^^^^^^^^^ required by this bound in `App::<'help>::subcommand`

For more information about this error, try `rustc --explain E0277`.
error: could not compile `sea-orm-cli` due to 2 previous errors

command.rs from clap 3

#[inline]
#[must_use]
pub fn subcommand<S: Into<App<'help>>>(mut self, subcmd: S) -> Self {
    self.subcommands.push(subcmd.into());
    self
}

You probably don't see that because clap version 2.34 is necessary for sea-schema

@smonv
Copy link
Contributor Author

smonv commented May 11, 2022

@cindRoberta thanks for pointing out, I missed the change in sea-schema.

I'll update more code with derive feature too.

@smonv smonv marked this pull request as draft May 11, 2022 02:45
@smonv
Copy link
Contributor Author

smonv commented May 12, 2022

@ikrivosheev I just update sea-orm-cli to new code structure using derive, maybe you can review it early.
Also I got some difficult while update testing code with new CLI interface and not sure how to test and make it pass. Can you help or give some insight about that part ?

Copy link
Member

@ikrivosheev ikrivosheev left a comment

Choose a reason for hiding this comment

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

@smonv thank you for contribution! Some comments

sea-orm-migration/src/cli.rs Outdated Show resolved Hide resolved
sea-orm-cli/src/migration.rs Show resolved Hide resolved
sea-orm-migration: bump clap version to 3.1.17

sea-orm-cli: use clap derive API instead of builder API

sea-orm-migration: use clap derive
@ikrivosheev
Copy link
Member

@smonv tests are passed. Can we review your PR or do you have some Todo?

@smonv smonv marked this pull request as ready for review May 16, 2022 00:01
@smonv
Copy link
Contributor Author

smonv commented May 16, 2022

@smonv tests are passed. Can we review your PR or do you have some Todo?

Yes, please help me review this. Sorry I update on last weekend so I forgot to request review.

Copy link
Member

@ikrivosheev ikrivosheev left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@billy1624 billy1624 left a comment

Choose a reason for hiding this comment

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

Hey @smonv, sorry for the delay. All in all, it looks good!

I found an issue with the updated migration CLI:

sea-orm/examples/tonic_example/migration on  pr/706 [$!] is 📦 v0.1.0 via 𝗥 v1.61.0 via 🅒 base
➜ DATABASE_URL="postgres://root:root@localhost/active_enum_tests" cargo r -- up -v
    Finished dev [unoptimized + debuginfo] target(s) in 0.20s
     Running `/Applications/MAMP/htdocs/sea-orm/examples/tonic_example/target/debug/migration up -v`
error: The argument '--verbose <VERBOSE>' requires a value but none was supplied

USAGE:
    migration up [OPTIONS]

For more information try --help

I updated the code and fixed some clippy warnings, feel free to review and cherrypick it onto this PR:

I changed the Option<bool> to bool for the verbose field. Just like what you did to the sea-orm-cli.

@smonv
Copy link
Contributor Author

smonv commented Jun 2, 2022

@billy1624 thanks for the help. I just cherry pick all your commit to this PR. Please help review it again.

@smonv smonv changed the title Bump clap version to 3.1.17 Bump clap version to 3.2 Jun 14, 2022
@smonv
Copy link
Contributor Author

smonv commented Jun 14, 2022

I just push changes needed to bump clap to 3.2

@tyt2y3
Copy link
Member

tyt2y3 commented Jun 14, 2022

@billy1624 looks good?

Copy link
Member

@billy1624 billy1624 left a comment

Choose a reason for hiding this comment

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

Thanks! @smonv Looks good!
CC @tyt2y3

@smonv
Copy link
Contributor Author

smonv commented Jun 15, 2022

Thanks! @smonv Looks good!
CC @tyt2y3

@tyt2y3 @billy1624 thank for helping me.

Happy to help!

@tyt2y3 tyt2y3 merged commit 580fa90 into SeaQL:master Jun 26, 2022
@tyt2y3
Copy link
Member

tyt2y3 commented Jun 26, 2022

Thank you all

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.

update clap to last version: clap = { version = "3.1.8",
5 participants