ArgMatches::value_of should include arguments to subcommands

[sgrif]

In Diesel CLI we are using a global argument in combination with subcommands. I'd expect matches.value_of("ARG") to return the argument regardless of where it appeared. However, in Diesel CLI (which is using clap 2.24.2), running diesel migration run --database-url something will complain that the argument isn't present. Running diesel migration --database-url something run works.

[kbknapp]

@sgrif apologies for the late reply, I've been out of town. Looking at the diesel source, this looks like a bug on clap's end. It should be a quick fix though. Let me see if I can get it put together today.

[kbknapp]

@sgrif can you add AppSettings::PropagateGlobalValuesDown and see if it still gives you an error?

Why isn't this the default you ask? It wasn't possible a long time ago, so it was added after the fact 😜

[sgrif]

I won't get to it for a few days, but yes I will see if that works. :)

[sgrif]

It's been a lot of days, but I finally got around to trying this. The setting improves things, but still doesn't fix the problem.

Command	diesel --db-url="" db drop	diesel db --db-url="" drop	diesel db drop --db-url=""
Without Setting	Arg not found	Arg not found	Fine
With Setting	Fine	Arg not found	Fine

Command	diesel --db-url="" migration run	diesel migration --db-url="" run	diesel migration run --db-url=""
Without Setting	Arg not found	Fine	Arg not found
With Setting	Fine	Fine	Arg not found

[kbknapp]

No worries, I've been incredibly busy too. Thanks for all the details, once I'm back at a computer (I'm on mobile) I should be able to figure this one out quickly

[willmurphyscode]

Do you know whether anyone is already working on this? If not, I'd like to try to fix it over the next few days.

[kbknapp]

@willmurphyscode

I get home from my travel tomorrow and was planning on checking it out then, but if you'd like to take a swing at it I'd be more than happy to mentor it and assist where needed. 👍

[willmurphyscode]

@kbknapp I'll take a swing at it. I confirmed the issue locally with diesel, so I was going to write some failing tests to start with. What's the best place to reach out if I need hand or have a question?

[willmurphyscode]

@kbknapp I've written some failing tests that show the issue: https://github.com/willmurphyscode/clap-rs/blob/propagate-values-down/tests/propagate_globals_down.rs#L52 but I'm a little bit lost trying to figure out exactly what to change needs to be made to make them pass. I've commented the tests in that link. Would you mind providing a little help when you have a chance?

[kbknapp]

@willmurphyscode

first_subcommand_can_access_global_arg_if_global_arg_is_last

Your hypothesis is correct which was initially by design.

To fix it, you'll have to bubble all these values back up just prior to validation occurring.

Some edge cases to watch out for would be program --global-arg some_value outer --global-arg other_value where multiple values aren't true. But this should just work because validation will occur after propagation.

second_subcommand_can_access_global_arg_if_global_arg_is_in_the_middle

You're probably right here too.

To solve it, you'll probably just need to add some recursive calls to ArgMatcher::propagate down through all subcommands.

[kbknapp]

I believe one or both of these cases are actually already fixed in 3.x just due to being a more clean code base...but I'll add this to the 3.x issue tracker just in case.

[willmurphyscode]

@kbknapp You say "one or both of these is fixed in 3.x." Does that mean that there's nothing to do here and people should just wait for 3.x? I've opened a PR with the tests (#1060) in case someone else wants to follow up, but beyond that I'm probably out of time to work on this issue.

[kbknapp]

You say "one or both of these is fixed in 3.x." Does that mean that there's nothing to do here and people should just wait for 3.x?

Since I'm not sure when I'll be able to finalize 3.x with my crazy work schedule right now, I'd rather fix them in 2.x in the mean time. I saw your comment in the PR about lack of time as well, so I can pick up where you left off. Thanks for starting this though 👍

[Freyskeyd]

@kbknapp I also can contribute to this issue.

I need some input to get started.

[kbknapp]

@Freyskeyd thanks! 👍

I would take the failing tests in #1060 and start with second_subcommand_can_access_global_arg_if_global_arg_is_in_the_middle as it will probably be easier to solve. It may be as easy as looping through all matched subcommands and recursively calling propagate.

If you run into any questions, feel free to ping me on Gitter since I'll get those notifications on my cell phone as well.

[kbknapp]

@willmurphyscode off topic, I saw your name on the upcoming Meetup this week I didn't know you were in the area! Looking forward to meeting you guys. 😄

[kbknapp]

#1070 fixes this. @sgrif could you test your use case against the current master branch? If it's good to go I can put out another version here soon.

[sgrif]

Yes, this resolves my issue

[sgrif]

Would love a ping when a new version is released. :)

[kbknapp]

@sgrif will do! The goal is to release before I leave for Rust Belt Rust 😉

[kbknapp]

@sgrif v2.27.0 is out on crates.io and contains these fixes :)