Allow bypassing negation

[bernard-wagner]

Pull Request

Problem

Sometimes it is desired to have an option starting with --no to be treated as a regular boolean argument. I encountered this when writing a wrapper for another command and needing to pass the options as is to the sub-process.

Solution

Since negate will be true for any option starting with --no, the default behavior can be overridden by explicitly changing negate to false.

[shadowspawn]

There have been some requests whether it is possible to disable the negate support: #1100 #2068

[shadowspawn]

I would like a couple more tests, but I can add these myself later if you prefer.

I would like a test that when the overridden flag is not specified that the option value is undefined rather than true.

And an explicit test, although redundant, that the name of the stored option is just camel case. So see noCheese in options and `cheese' is undefined. This one is implicitly tested already, but since it is a key change I would like an explicit test covering this particular behaviour.

[bernard-wagner]

I would like a couple more tests, but I can add these myself later if you prefer.

I would like a test that when the overridden flag is not specified that the option value is undefined rather than true.

And an explicit test, although redundant, that the name of the stored option is just camel case. So see noCheese in options and `cheese' is undefined. This one is implicitly tested already, but since it is a key change I would like an explicit test covering this particular behaviour.

I think the tests cover all the permutations now.

[shadowspawn]

Thanks for all the tests for a small code change! I did need to search the code a couple of times to check the one-liner was really going to work, so the tests are good for confirming and also catching future regressions.

One more variation. You added this:

  test('when negatable boolean flag is specified and negate is overridden then positive value is undefined', () => {

And I would also like this variation, to check the default is not getting set. This is implicitly tested by the above, but the default value being true for a negated boolean is a feature of the standard behaviour and I want to check it does not occur.

  test('when negatable boolean flag is unspecified and negate is overridden then positive value is undefined', () => {

[shadowspawn]

One more test suggested, but not needed for approval.

[shadowspawn]

(Thanks for the work on the extra tests @bernard-wagner )

Having second thoughts!

[shadowspawn]

I am having second thoughts about this @abetomo

Basically, I don't want to commit to supporting bypassing negation as a feature, like by adding a chaining method on Option to clear negate. So I don't really want to add all the tests showing we support it (although they were good for showing it does work currently).

Two possible approaches for your consideration:

1. we could just change attributeName implementation as in this PR and not add any tests. So it would make it easier to bypass negation without us officially supporting this as a use case. And I think the change is reasonable on its own, makes the code a little clearer.
2. we could add an example file showing subclassing Option to allow bypassing negation, including a chaining method. (With a note that it isn't officially supported.)

(Edit: I am leaning towards option 1.)

[abetomo]

I too think the 1 approach is better.

[shadowspawn]

Closing in favour of #2250 which has just the code change and not the tests.

Thanks for the suggestion @bernard-wagner and work on the tests.