negated boolean value handled differently in 4.7.0

[sebhoss]

Not sure if this was done intentionally but I could not find anything in the release notes thus this ticket.

The following test cases work fine in 4.6.3 but not entirely in 4.7.0:

import org.junit.jupiter.api.Test;
import picocli.CommandLine;

import static org.junit.jupiter.api.Assertions.*;

public class PicocliBooleanTest {

  @CommandLine.Command
  static class TestCommand implements Runnable {

    @CommandLine.Option(
        names = {"--no-interactive"},
        defaultValue = "true",
        negatable = true
    )
    public boolean interactive;

    @Override
    public void run() {}
  }

  @Test
  public void optionIsTrueByDefault() {
    final var commandLine = new CommandLine(new TestCommand());
    final var parseResult = commandLine.parseArgs();
    final var command = (TestCommand) parseResult.commandSpec().userObject();
    assertTrue(command.interactive);
  }

  @Test
  public void optionCanBeSetToFalse() {
    final var commandLine = new CommandLine(new TestCommand());
    final var parseResult = commandLine.parseArgs("--interactive=false");
    final var command = (TestCommand) parseResult.commandSpec().userObject();
    assertFalse(command.interactive);
  }

}

The second test case uses --interactive=false which does not seem to work anymore on a negatable option.

[remkop]

Did this used to work differently?
I tried these additional tests from your base and it looks okay to me but maybe I am missing something...

@Test
public void optionIsTrueByDefault() {
    final CommandLine commandLine = new CommandLine(new TestCommand());
    final CommandLine.ParseResult parseResult = commandLine.parseArgs();
    final TestCommand command = (TestCommand) parseResult.commandSpec().userObject();
    assertTrue(command.interactive);
}

@Test
public void optionFalseWhenSpecified() {
    final TestCommand command = new TestCommand();
    new CommandLine(command).parseArgs("--no-interactive");
    assertFalse(command.interactive);
}

@Test
public void optionTrueWhenNegatedFormSpecified() {
    final TestCommand command = new TestCommand();
    new CommandLine(command).parseArgs("--interactive");
    assertTrue(command.interactive);
}

@Test
public void optionCanBeSetToFalse() {
    final TestCommand command = new TestCommand();
    //CommandLine.tracer().setLevel(CommandLine.TraceLevel.DEBUG);
    new CommandLine(command).parseArgs("--no-interactive=false");
    assertFalse(command.interactive);
}

@Test
public void optionNegatedFormSetToFalseIsTrue() {
    final TestCommand command = new TestCommand();
    new CommandLine(command).parseArgs("--interactive=false");
    assertTrue(command.interactive);
}

[sebhoss]

yeah your last test works differently in 4.6

--interactive=false now sets interactive to true but used to set it to false. Likewise, setting --interactive=true now results in false but used to be true. I think that's odd because --interactive still results in true.

[sebhoss]

Created a slightly larger test case that covers negatable options with and without --no-.. and using different default values. The tests all work in 4.7 and I marked the ones failing in 4.6 with a comment:

  @CommandLine.Command
  static class TestCommand implements Runnable {

    @CommandLine.Option(
        names = {"--no-interactive"},
        defaultValue = "true",
        negatable = true
    )
    public boolean interactive;

    @CommandLine.Option(
        names = {"--no-autonomous"},
        defaultValue = "false",
        negatable = true
    )
    public boolean autonomous;

    @CommandLine.Option(
        names = {"--daemon"},
        defaultValue = "true",
        negatable = true
    )
    public boolean daemon;

    @CommandLine.Option(
        names = {"--human"},
        defaultValue = "false",
        negatable = true
    )
    public boolean human;

    @Override
    public void run() {}

  }

  @Test
  public void optionNotSpecified() {
    final TestCommand command = new TestCommand();
    new CommandLine(command).parseArgs();
    assertAll(
        () -> assertTrue(command.interactive, "interactive"),
        () -> assertFalse(command.autonomous, "autonomous"),
        () -> assertTrue(command.daemon, "daemon"),
        () -> assertFalse(command.human, "human")
    );
  }

  @Test
  public void optionSpecifiedWithoutValue() {
    final TestCommand command = new TestCommand();
    new CommandLine(command).parseArgs("--interactive", "--autonomous", "--daemon", "--human");
    assertAll(
        () -> assertTrue(command.interactive, "interactive"),
        () -> assertFalse(command.autonomous, "autonomous"),
        () -> assertFalse(command.daemon, "daemon"),
        () -> assertTrue(command.human, "human")
    );
  }

  @Test
  public void optionSpecifiedWithBooleanTrue() {
    final TestCommand command = new TestCommand();
    new CommandLine(command).parseArgs("--interactive=true", "--autonomous=true", "--daemon=true", "--human=true");
    assertAll(
        () -> assertFalse(command.interactive, "interactive"), // FAIL in 4.6
        () -> assertFalse(command.autonomous, "autonomous"), // FAIL in 4.6
        () -> assertTrue(command.daemon, "daemon"),
        () -> assertTrue(command.human, "human")
    );
  }

  @Test
  public void optionSpecifiedWithBooleanFalse() {
    final TestCommand command = new TestCommand();
    new CommandLine(command).parseArgs("--interactive=false", "--autonomous=false", "--daemon=false", "--human=false");
    assertAll(
        () -> assertTrue(command.interactive, "interactive"), // FAIL in 4.6
        () -> assertTrue(command.autonomous, "autonomous"), // FAIL in 4.6
        () -> assertFalse(command.daemon, "daemon"),
        () -> assertFalse(command.human, "human")
    );
  }

  @Test
  public void optionSpecifiedWithNegatedForm() {
    final TestCommand command = new TestCommand();
    new CommandLine(command).parseArgs("--no-interactive", "--no-autonomous", "--no-daemon", "--no-human");
    assertAll(
        () -> assertFalse(command.interactive, "interactive"),
        () -> assertTrue(command.autonomous, "autonomous"),
        () -> assertTrue(command.daemon, "daemon"),
        () -> assertFalse(command.human, "human")
    );
  }

  @Test
  public void optionSpecifiedWithNegatedFormUsingBooleanTrue() {
    final TestCommand command = new TestCommand();
    new CommandLine(command).parseArgs("--no-interactive=true", "--no-autonomous=true", "--no-daemon=true", "--no-human=true");
    assertAll(
        () -> assertTrue(command.interactive, "interactive"),
        () -> assertTrue(command.autonomous, "autonomous"),
        () -> assertFalse(command.daemon, "daemon"), // FAIL in 4.6
        () -> assertFalse(command.human, "human") // FAIL in 4.6
    );
  }

  @Test
  public void optionSpecifiedWithNegatedFormUsingBooleanFalse() {
    final TestCommand command = new TestCommand();
    new CommandLine(command).parseArgs("--no-interactive=false", "--no-autonomous=false", "--no-daemon=false", "--no-human=false");
    assertAll(
        () -> assertFalse(command.interactive, "interactive"),
        () -> assertFalse(command.autonomous, "autonomous"),
        () -> assertTrue(command.daemon, "daemon"), // FAIL in 4.6
        () -> assertTrue(command.human, "human") // FAIL in 4.6
    );
  }

[remkop]

Thank you for this extensive test! Very helpful!
I found the change in 4.6.0 that causes this: #1642

Do you think that the old 4.6.0 behaviour is more intuitive than the 4.7 behaviour?

[sebhoss]

The expected behavior in #1642 is the same as the --human option in the above test cases and I think that works fine in 4.7.0, e.g.:

--human          // Value: true
--no-human       // Value: false
--human=true     // Value: true
--no-human=true  // Value: false (changed in 1642)
--human=false    // Value: false
--no-human=false // Value: true (changed in 1642)

However, a negatable option that is true by default looks weird to me:

--interactive          // Value: true
--no-interactive       // Value: false
--interactive=true     // Value: false (changed in 1642)
--no-interactive=true  // Value: true
--interactive=false    // Value: true (changed in 1642)
--no-interactive=false // Value: false

I think the major reason this looks weird to me is that in my mind --no-.. was always the negated form but in the above example --interactive is the negated form because it was initially declared as names = {"--no-interactive"}. From a users perspective this seems confusing to me, e.g. why does --interactive result in true but --interactive=true is false? Changing the result of --interactive=true is weird as well because it would be the same as --no-interactive=true in the current model. I think a better user experience would be the following which does require the change from #1642 :

--interactive          // Value: true
--no-interactive       // Value: false
--interactive=true     // Value: true (!)
--no-interactive=true  // Value: false (!)
--interactive=false    // Value: false (!)
--no-interactive=false // Value: true (!)

This treats --interactive=true the same as --interactive and --interactive=false the same as --no-interactive. With the changes in 4.7.0 I was able to get it working as I want with this:

  @CommandLine.Command
  static class TestCommand implements Runnable {

    @CommandLine.Option(
        names = {"--wanted"}, // this changed
        defaultValue = "true",
        fallbackValue = "true", // this is new
        negatable = true
    )
    public boolean wanted;

    @Override
    public void run() {}

  }

  @Test
  public void optionNotSpecified() {
    final var command = new TestCommand();
    new CommandLine(command).parseArgs();
    assertTrue(command.wanted, "wanted");
  }

  @Test
  public void optionSpecifiedWithoutValue() {
    final var command = new TestCommand();
    new CommandLine(command).parseArgs("--wanted");
    assertTrue(command.wanted, "wanted");
  }

  @Test
  public void optionSpecifiedWithBooleanTrue() {
    final var command = new TestCommand();
    new CommandLine(command).parseArgs("--wanted=true");
    assertTrue(command.wanted, "wanted");
  }

  @Test
  public void optionSpecifiedWithBooleanFalse() {
    final var command = new TestCommand();
    new CommandLine(command).parseArgs("--wanted=false");
    assertFalse(command.wanted, "wanted");
  }

  @Test
  public void optionSpecifiedWithNegatedForm() {
    final var command = new TestCommand();
    new CommandLine(command).parseArgs("--no-wanted");
    assertFalse(command.wanted, "wanted");
  }

  @Test
  public void optionSpecifiedWithNegatedFormUsingBooleanTrue() {
    final var command = new TestCommand();
    new CommandLine(command).parseArgs("--no-wanted=true");
    assertFalse(command.wanted, "wanted");
  }

  @Test
  public void optionSpecifiedWithNegatedFormUsingBooleanFalse() {
    final var command = new TestCommand();
    new CommandLine(command).parseArgs("--no-wanted=false");
    assertTrue(command.wanted, "wanted");
  }

This deviates from the docs about negatable options that are true by default but I think it's nicer for users. Feel free to close this one & thanks for helping!

[remkop]

I finally had time to think this through. Nice! Your solution is much better than the current recommendation in the manual. The current recommendation of defining the option as --no-xxx with default value true does not work well when the end user specifies an option parameter, as you point out.

I will update the user manual to recommend your solution instead:

When a negatable option is true by default, give it both a defaultValue and a fallbackValue of "true". For example:

@Option(names = "--backup", negatable = true,
     defaultValue = "true", fallbackValue = "true",
     description = "Make a backup. True by default.")
boolean backup;

[remkop]

Done. The docs have been updated.
Thank you for the discussion! 🙏