Cargo: Build CLAP command using derive #213
Conversation
xFrednet
added
C-enhancement
xFrednet
force-pushed
the
213-derive-clap
branch
from
f2684ab to
a2ef988
Compare
xFrednet
force-pushed
the
213-derive-clap
branch
from
a0bc857 to
cfe11a3
Compare
|
Good catches, thank you very much for the review! You can merge this PR by commenting bors delegate=NathanFrasier |
|
✌️ NathanFrasier can now approve this pull request. To approve and merge a pull request, simply reply with |
| let backend_conf = backend::Config { | ||
| dev_build: flags.dev_build, | ||
| dev_build: cfg!(debug_assertions), |
There was a problem hiding this comment.
IIRC there are several places where dev build flag influences the discovery of the toolchain and driver.
I'd prefer if there was a magic env variable e.g. MARKER_DEV_MODE=1 that could be used for conditions when a dev mode is used. This wouldn't require building in release or changing code temporarily to [de]activate the dev mode.
There was a problem hiding this comment.
Part of this change was to have a unified way across the entire project to enter dev mode. Having an environment value could work, but would be a bit weird, as it would always need to be set when interacting with the project. Or am I missing another option I'm missing?
There was a problem hiding this comment.
I guess you are right. Maybe MARKER_RELEASE_MODE could then be used as an override to opt out.
There was a problem hiding this comment.
I feel like it's too easy for a user to accidentally install it without setting the environment value.
|
I want to combine the two review commits, before the merge. I'll wait a bit more for more feedback :) |
There was a problem hiding this comment.
I happened to stumble upon a project called cargo-nextest. It also uses clap derive for its CLI. That project doesn't make a special case for running cargo-nextest directly. It requires you to specify cargo-nextest nextest as a subcommand (CLI code). Maybe that code could be helpful
| assert!(matches!(cli.command, Some(CliCommand::Check(_)))); | ||
|
|
||
| let cli = MarkerCli::parse_from(&["cargo-marker"]); | ||
| assert!(matches!(cli.command, None)); |
There was a problem hiding this comment.
I think you may want an assert_maches crate. expect_test could also fit
There was a problem hiding this comment.
The assert_maches link points to the CLI code of next test and I can't find the create on crates.io
Something like this would definitely be useful. You already mentioned this in a previous PR and #202 exists to investigate it. I would rather not add it in this PR, to keep it more contained, but this would be a great application for it 👍 (I just need time to get through all the issues 😅 )
There was a problem hiding this comment.
Yeah, an apology for the typo. It's assert_matches
There was a problem hiding this comment.
I think expect_test would be a better fit here, mostly because it allows automatic test updates. Still, I won't add it in this PR. My next weeks will be more busy, and I want to make sure this and the other stuff is added before 0.2.0.
I really like this idea, thank you for the suggestion! |
xFrednet
force-pushed
the
213-derive-clap
branch
from
f87ca41 to
4644eb5
Compare
|
I've squashed the review commits, this should not be ready to be merged. bors r=Veetaha |
|
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page.
|
Just a simple refactoring. At first, I was a bit hesitant, but I believe that this is the better way to handle Marker's CLI. Only the
--test-setupargument changed into a hidden subcommand, the rest should be the same as before.Nothing more to say otherwise. Reviews are appreciated. For everyone reading this, have a beautiful day! :D