fix: support --option=false syntax for boolean flags (#2504) - #2505
Conversation
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@stalep could you add the following 2 tests to
Edit: Duh, I'm already running the latest version with AEsh (and therefore this issue) included of course Edit2: or should we add it to |
|
Added both tests to |
…ngdev#2504) Make inherited global flags (verbose, quiet, offline, fresh, preview, stacktrace) negatable so --no-verbose, --no-offline etc. can be used to override config-file defaults. Use Boolean wrappers so afterParse() can distinguish 'not specified' (null) from 'explicitly false'. For example: jbang config set verbose true jbang --no-verbose version # overrides config, runs quietly Add unit tests for --no-verbose, --no-offline, --no-fresh and integration tests verifying config override with --no-verbose.
|
tests are about |
Indeed, I know it's an options that Picocli supports, but I thought we never used/enabled it |
|
Besides --no-verbose might not be really what we want it does seem to reveal an issue with the afterParse() - its called twice...causing overrides that isn't right. Detailed analysis below: Build Failure AnalysisFailing test: What the test does
Root cause
The execution flow for
The subcommand's inherited Possible fixes
|
|
asked if exist on main - something is not right having negatable verbose. The double afterParse call pattern exists on main too — both JBang (parent) and Version (child) get afterParse() called, and the child's verbose field gets the config value from JBangDefaultValueProvider. But it's NOT a bug on main because the old logic is one-directional: // main — can only ENABLE, never DISABLE
if (verbose) { Util.setVerbose(true); }This means:
There's no way to trigger the bug on main because:
The PR exposes this latent design issue by making the flags bidirectional (Boolean + negatable). Now the child's afterParse can actively set Util.setVerbose(true) from the config default, overriding the parent's |
Proposed fixCan this be right? Seems overly complex :) The fix is to skip config defaults for inherited options on child commands in diff --git a/src/main/java/dev/jbang/cli/JBangDefaultValueProvider.java b/src/main/java/dev/jbang/cli/JBangDefaultValueProvider.java
index 8530d097..95d7ee9c 100644
--- a/src/main/java/dev/jbang/cli/JBangDefaultValueProvider.java
+++ b/src/main/java/dev/jbang/cli/JBangDefaultValueProvider.java
@@ -33,6 +33,18 @@ public class JBangDefaultValueProvider implements DefaultValueProvider {
return null;
}
+ // Skip config defaults for inherited options on child commands.
+ // The root command (JBang) will get the config default and handle
+ // it in afterParse(). Without this guard, a child command's config
+ // default (e.g. verbose=true) would override a CLI flag like
+ // --no-verbose that was already processed by the parent.
+ if (option.isInherited()
+ && option.parent() != null
+ && option.parent().getCommand() != null
+ && !(option.parent().getCommand() instanceof JBang)) {
+ return null;
+ }
+
String optName = option.name().replace("-", "");
String fullPath = null;Tested locally — all |
Prevents child command's config default (e.g. verbose=true) from overriding a CLI flag like --no-verbose already processed by the parent in afterParse().
|
Applied your fix for |
Use Boolean wrappers instead of boolean primitives for inherited global flags (verbose, quiet, offline, fresh, preview, stacktrace). This allows afterParse() to distinguish between 'not specified' (null) and 'explicitly set to false' (Boolean.FALSE), so that --offline=false correctly overrides a config-file default of true.
Previously the primitive boolean fields defaulted to false, making it impossible to tell if the user explicitly passed --offline=false or simply didn't specify the flag at all.