Recursive needs processing #1212
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1212 +/- ##
===========================================
- Coverage 100.00% 99.90% -0.10%
===========================================
Files 17 19 +2
Lines 4546 5251 +705
Branches 0 1067 +1067
===========================================
+ Hits 4546 5246 +700
- Misses 0 5 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
VolkerChristian
force-pushed
the
Recursive_needs_requirement_check
branch
4 times, most recently
from
709c1d0 to
a0d9aa4
Compare
The current way needs relations are checked in void App::_process_requirements()
is missleading.
Assume you have an application 'test' with a CLI11 config system configuring
following needs relations between some subcommands:
app ---> sub1 ---> sub11
| |-> sub12
|-> sub2
A) Current needs processing gives:
./test -> test requires sub1 // OK for the user
./test sub1 -> test requires sub2 // OK for the user
./test sub1 sub2 -> sub1 requires sub11 // Confuses the user
./test sub1 sub2 sub11 -> ready for execution
B) Recursive needs processing gives:
./test -> test requires sub1 // OK
./test sub1 -> sub1 requires sub11 // Much more intuitive for the user
./test sub1 sub2 -> sub1 requires sub11 // Makes sense: sub1 configuration is not compleate yet
./test sub1 sub11 -> test requires sub2 // OK, sub1 configuration is compleate
./test sub1 sub11 sub2 -> ready for execution
VolkerChristian
force-pushed
the
Recursive_needs_requirement_check
branch
from
a0d9aa4 to
d32c0b3
Compare
| missing_need = opt->get_name(); | ||
| } | ||
| } | ||
| for(const auto &subc : need_subcommands_) { // Process subcommands given on the commandline first |
There was a problem hiding this comment.
So you want to check the subcommands that are needed and were given and make sure all their requirements are met first. This is to get at the lower level errors with priority.
| try { | ||
| subc->_process_requirements(); | ||
| } catch (const CLI::RequiresError&) { | ||
| throw RequiresError(get_display_name(), subc->name_); |
There was a problem hiding this comment.
next you are going to check the subcommands that were not parsed and give priority to missing requirements in those needed subcommands, and make the error clearer.
include/CLI/impl/App_inl.hpp
Outdated
| } | ||
| } | ||
| if(sub->count() > 0 || sub->name_.empty()) { | ||
| if(sub->count() > 0 || sub->name_.empty() && need_subcommands_.find(sub.get()) == need_subcommands_.end()) { |
There was a problem hiding this comment.
this check is to process the requirements for the subcommands that were given but were not in the needed list
There was a problem hiding this comment.
I think this does need another set of parenthesis around the original 2 conditions, otherwise the logic is odd. And I think that is why the test cases are failing.
|
I can see this being reasonable and cleaning up some of the error messages in specific situations. Can we add some test cases, figure out what is going on with the existing test cases and we can merge it. |
Fine that you want to integrate this behavior. Nevertheless, I am extremely busy currently and I don't know when there is enough time to add the test cases and to have a deeper look at the now failed test about option groups. I want to suggest to postpone this patch and not to integrate it in the next release as I don't want to delay the release. |
The current way needs relations are checked in void App::_process_requirements() is missleading.
Assume you have an application 'test' with a CLI11 config system configuring following needs relations between some subcommands:
A) Current needs processing gives:
B) Recursive needs processing gives: