Shared initialized set #1254
Shared initialized set #1254
Conversation
remkop
left a comment
remkop
left a comment
There was a problem hiding this comment.
I finally had a chance to review the PR in detail. First of all, many thanks!
With a small change, all tests pass: in processSubcommand, instead of passing the initialized set as is to the subcommand, we pass an empty list and use that to collect the args (and associated original/ancestor args) that were initialized by the subcommand. If we use this set for validation and setting defaults in the parent command, it works as intended. See my comment on that change for details.
If you make that change the PR can be merged. It would be good if we can have additional tests for named options similar to SubcommandTest::testInheritedParameter.
I also have an idea to improve the implementation even further: instead of doing a best-effort lookup with our findOriginal method, we can add a ancestor() method to the ArgSpec class that returns the original for an inherited ArgSpec (and null for non-inherited args). See my comment on that change for details. We can either make that improvement part of this PR if you want, or do this separately. Please let me know if you are interested in working on that.
|
First of all, @remkop thanks a lot for the work you put into reviewing this PR, and for doing it so quickly! You have been one of the most responsive maintainers that I have interacted with, and had several great ideas! I'm in a bit of a tight stretch work-wise, I'll try to have a look at your suggestions tomorrow or the day after, but I am interested in implementing the improvements you mention. |
|
@danielthegray That is great, thanks for helping out with this! Thinking about some details that may come up during the implementation:
|
FYI, this causes other tests to start failing, so it's not a full solution
danielthegray
force-pushed
the
shared_initialized_set
branch
from
3eea804 to
12b26df
Compare
Codecov Report
@@ Coverage Diff @@
## master #1254 +/- ##
============================================
- Coverage 93.81% 93.68% -0.13%
Complexity 458 458
============================================
Files 2 2
Lines 6753 6789 +36
Branches 1823 1835 +12
============================================
+ Hits 6335 6360 +25
- Misses 126 129 +3
- Partials 292 300 +8
Continue to review full report at Codecov.
|
|
I saw you pushed a change. Do you want me to merge this, and do any further changes in a separate PR? |
The best-effort matching search to add the root element was changed to simply wire up the root ArgSpec to each of the inherited arguments. Arguments which are not inheritable will have a null root, and inheritable arguments (options or positional parameters) will have a root() method that returns the root argument, or the argument itself if it is already at the root. Unit tests have also been added to check that this behavior is maintained.
|
I essentially rebased the previous work off master (now that the annoying line endings problem was gone, making it easier to switch branches without being left in an un-workable state). I have implemented your suggestion, with a small change which I will detail in the relevant review thread, to allow for infinitely nested subcommands. Let me know what your thoughts are! From my side, this is ready to merge. |
I missed this during my last check, I will add these as well. |
|
Merged with some minor changes. |
|
@remkop Thank you very much! That was a great catch, that the |
|
Ok, can you paste a diff/patch of your change here? Since this PR has already been merged I don't think it is possible to merge any subsequent changes. |
|
Sure! diff --git a/src/main/java/picocli/CommandLine.java b/src/main/java/picocli/CommandLine.java
index 02dd44c2..ca21e508 100644
--- a/src/main/java/picocli/CommandLine.java
+++ b/src/main/java/picocli/CommandLine.java
@@ -9060,7 +9060,6 @@ public class CommandLine {
public T hidden(boolean hidden) { this.hidden = hidden; return self(); }
/** Sets whether this option is inherited from a parent command, and returns this builder.
- * <b>Do not forget to also set {@link #root()} when setting this, to ensure that it always can track back to the root element.</b>
* @since 4.3.0 */
public T inherited(boolean inherited) { this.inherited = inherited; return self(); }
|
|
Merged. Thank you for the attention to details! |
|
Thank you again for getting this merged so quickly! |
3 participants
This was an attempt to fix #1250 following the proposal there, but this causes other test failures. I am leaving this on a branch/PR for further examination if you think it is salvageable.