Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Negatable boolean @Option with default value "true" is not negated as expected #813

Closed
YannRobert opened this issue Sep 16, 2019 · 7 comments
Milestone

Comments

@YannRobert
Copy link

Hi

I have a boolean Option that I want to be true by default.
I also want the user to be able to set this option to false.

I want the Option name to be explicit enough to be self-documenting (actual code provides a description though)
The --(no-)option modifier seems like a perfect fit at first. Even if --option is redundant with default value.

In order to do this, I use the following code :

	@CommandLine.Option(
			names = "--demote-first",
			defaultValue = "true",
			negatable = true
	)
	private boolean demoteFirst;

Using latest version as of now (version 4.0.4)

When default value is true :

  • using the negative modifier of a boolean Option does not change the value to false.
  • using the Option does change the value to false

I think this is counter intuitive.
Is this the expected behavior ?

In order to avoid this, I have to change the code to :

make the Option not negatable (this is my favorite)

	@CommandLine.Option(
			names = "--no-demote-first",
			defaultValue = "true"
	)
	private boolean demoteFirst;

or

make the Option negatable and also required (without any default value otherwise required is ignored)

	@CommandLine.Option(
			names = "--demote-first",
			//defaultValue = "true",
			negatable = true,
			required = true
	)
	private boolean demoteFirst;

For Options with a false default value, we do not observe this.

This means that usage is not consistant depending on the default value.

remkop added a commit that referenced this issue Sep 16, 2019
@remkop remkop added this to the 4.0.5 milestone Sep 16, 2019
@remkop
Copy link
Owner

remkop commented Sep 16, 2019

You raise a good point. Double negatives are always a source of confusion...

If your option is called --no-demote-first, then its "negated form" is --demote-first. The negated form sets the default value. The original form (--no-demote-first) sets the opposite of the default value.

Inspired by your feedback, I updated the user manual: https://picocli.info/#_negatable_options
Can you take a look and let me know if that is sufficient?

@remkop
Copy link
Owner

remkop commented Sep 17, 2019

@YannRobert Sorry if I wasn't clear.

You can still make the option negatable, even if you give it the name --no-demote-first:

@Option(names = "--no-demote-first", defaultValue = "true", negatable = true)
private boolean demoteFirst;

When the user specifies --no-demote-first, the value is set to false (the opposite of the default). When the user specifies the negated form --demote-first, the value is set to true (the default).

I believe this is how it should work. The key is to name the option --no-xxx when the default value is "true".

Does that make sense?

@remkop
Copy link
Owner

remkop commented Sep 19, 2019

@YannRobert I added this test. I believe it is working as expected.

@Test
public void testIssue813() {
    class App {
        @Option(names = "--no-xxx", defaultValue = "true", negatable = true)
        boolean xxx;

        @Option(names = "--yyy", defaultValue = "false", negatable = true)
        boolean yyy;
    }

    App app = new App();
    new CommandLine(app).parseArgs("--xxx", "--yyy");
    assertTrue(app.xxx);
    assertTrue(app.yyy);

    new CommandLine(app).parseArgs("--no-xxx", "--no-yyy");
    assertFalse(app.xxx);
    assertFalse(app.yyy);
}

Is this useful to help resolve the issue?

@YannRobert
Copy link
Author

Hi @remkop

I think your addition to the documentation is helping very much.

Also you additional test is helping,

but I would complete the test like this, maybe :

	@Test
	public void testIssue813_you_probably_should_not_use_it_this_way() {
		// see doc : Negatable options that are true by default
		class AppDont {
			@CommandLine.Option(names = "--zzz", defaultValue = "true", negatable = true)
			boolean zzz;
		}

		AppDont app;

		app = new AppDont();
		new CommandLine(app).parseArgs();
		assertTrue(app.zzz);

		app = new AppDont();
		new CommandLine(app).parseArgs("--zzz");
		assertFalse(app.zzz); // don't expect true

		app = new AppDont();
		new CommandLine(app).parseArgs("--no-zzz");
		assertTrue(app.zzz); // don't expect false

	}

	@Test
	public void testIssue813_but_you_probably_should_do_this() {
		// see doc : Negatable options that are true by default
		class AppDo {
			@CommandLine.Option(names = "--no-zzz", defaultValue = "true", negatable = true)
			boolean zzz;
		}

		AppDo app;

		app = new AppDo();
		new CommandLine(app).parseArgs();
		assertTrue(app.zzz);

		app = new AppDo();
		new CommandLine(app).parseArgs("--zzz");
		assertTrue(app.zzz);

		app = new AppDo();
		new CommandLine(app).parseArgs("--no-zzz");
		assertFalse(app.zzz);

	}

@remkop
Copy link
Owner

remkop commented Sep 19, 2019

@YannRobert Excellent, glad to hear that!
I will add your suggested tests when I get a chance.

If you like picocli, please star the project on GitHub and tell your friends! 😅

@remkop remkop closed this as completed Sep 19, 2019
@remkop
Copy link
Owner

remkop commented Sep 19, 2019

P.S. Your contribution is mentioned in the release notes for picocli 4.0.5.

@YannRobert
Copy link
Author

Thanks 👍

YannRobert added a commit to Credit-Agricole-Payment-Services/xap-operation-tool that referenced this issue Jan 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants