Implement most of the Diesel CLI for 0.4.0 #79
Conversation
|
@mfpiccolo @samphippen @mcasper Review? |
sgrif
force-pushed
the
sg-diesel-cli
branch
2 times, most recently
from
0781243 to
8980650
Compare
| .unwrap(); | ||
| } | ||
| ("redo", Some(args)) => { | ||
| let connection = connection(&database_url(args)); |
There was a problem hiding this comment.
this branch is at a different level of abstraction than the others. What do you think about extracting this into a function also?
There was a problem hiding this comment.
If anything I'm actually thinking of moving the other branches down. I don't like having #[doc(hidden)] public functions in the main library, and really the only "public" use case I can see is running pending migrations as part of your build script.
There was a problem hiding this comment.
given that this is main.rs, are any of the things in here actually public?
There was a problem hiding this comment.
No, that's what I mean. I'd rather move more of the logic into here and out of diesel proper.
There was a problem hiding this comment.
ah ok. Either way, this is mostly fine, it just caught me as a little odd that the branches weren't balanced. Do with that information what your much better informed than me self wants to do :)
|
Code looks good to me. What are the steps for running the commands from a project directory? |
|
You can cargo install it On Fri, Jan 8, 2016, 12:04 AM Mike Piccolo [email protected] wrote:
|
This adds all the plumbing, and 3 of the 4 commands that I want to ship with the CLI for 0.4.0. This adds the ability to run, revert, and redo migrations. There's still a lot of things I'd like to improve about the CLI. We need to give better error output in cases where something unexpected happens, and I'd like to proxy unknown subcommands to `diesel-subcommand` (this last one appears to be [a limitation of clap](clap-rs/clap#372). I've opted to make `diesel_cli` be a separate crate, as cargo doesn't allow you to declare dependencies as only for executables. I don't want to add `clap` or `chrono` (which I'll be adding as a dependency in the follow up to this commit) to become hard dependencies of diesel itself. Another unrelated enhancement I'd like to eventually make is adding `dotenv` support. I think this should be optional, but if it doesn't become a hard dependency of diesel itself, maybe it doesn't matter?
This adds all the plumbing, and 3 of the 4 commands that I want to ship
with the CLI for 0.4.0. This adds the ability to run, revert, and redo
migrations.
There's still a lot of things I'd like to improve about the CLI. We need
to give better error output in cases where something unexpected happens,
and I'd like to proxy unknown subcommands to
diesel-subcommand(thislast one appears to be a limitation of
clap.
I've opted to make
diesel_clibe a separate crate, as cargo doesn'tallow you to declare dependencies as only for executables. I don't want
to add
claporchrono(which I'll be adding as a dependency in thefollow up to this commit) to become hard dependencies of diesel itself.
Another unrelated enhancement I'd like to eventually make is adding
dotenvsupport. I think this should be optional, but if it doesn'tbecome a hard dependency of diesel itself, maybe it doesn't matter?