Switch from BoolVar to UnNegatableBoolVar everywhere to avoid confusion

[alexbozhenko]

Proposed change

BoolVar may confuse: Is the flag on or off by default? If it is on by default, the flag to enable is meaningless.

# nats audit gather -h                                                                                                        
...
      --[no-]progress           Display progress messages during gathering
      --[no-]server-endpoints   Capture monitoring endpoints for each server

There are 36 total cases where BoolVar is used, arcoss 16 commands.

# rg   'Default\("true"\).*BoolVar'     | less -N
# rg -l  'Default\("true"\).*BoolVar'    | sort | uniq | wc -l
16

from @jnmoyne in slack:

so, just to nail this down once and for all and make sure we are on the same page, let’s say taking for a simple example --no-progress in nats bench which right now is:

Flag("progress", "Enable or disable the progress bar").Default("true").BoolVar(&c.progressBar)

and therefore shows in the help as

      --[no-]progress           Enable or disable the progress bar

The right way that we want to change to at some point would be to:

1. rename the var to noProgressBar
2. Change to Flag("no-progress", "Enable or disable the progress bar").UnNegatableBoolVar(&c.noProgressBar)
3. switch the logic around in the code, i.e. from if (c.progressBar) to if(!c.noProgressBar

We already have 8 places where we use UnNegatableBoolVar like this.

# rg 'Flag.*no-.*BoolVar' | less -NS
8

This would be a change similar to
#1417 and #506.
When this is done, all the ambiguity around bool flags will be gone: we will have either --thing or --no-thing flags.

Use case

Added consistency and clarity.

Contribution

I can.

[ripienaar]

It's a fair bit more complex than this, as we have a 3rd case of negatable boolean - those where they are used to enable OR disable features.

For example you want to edit a stream and disable direct get, this means we need a --no-direct-get that defaults to false. Likewise we need a --direct-get that defaults to false (so you can enabled it) and this combines with IsSetByUser() to detect when these, essentially triple state booleans, need to active.

Here's an example PR that addresses this by only adding the --no-x variants on edit command, but in doing so VERY careful testing had to be done because doing too much such as if we wanted to do that with headers only on consumers would result in users not being able to disable interactive prompts other than with --defaults. The ability to pass --defaults was quite recently added and I know several users have scripts relying on --no-headers-only to disable the prompt. #1415

This is but one of the many many edge cases that we face when changing the behaviour of defaults and so for this reason we have to be very careful, very meticulous and whenever we change a field/flag behaviour ensure there are test coverage. I found the headers issue described here thanks to existing tests. nats auth though has a bit of a short coming in tests and so we would need to put that in first before changing it.

Testing is a ongoing effort, we've over the last months added about 4500 lines of tests to make it safer to do this kind of big all-touching changes, we're not there yet.

So step one here is unfortunately to flesh out testing sufficiently to be able to make this changes

[alexbozhenko]

Got it. Thank you for the write-up!
In, #1417, can I revert the auth change, so we can land --[no-]doubleack in nats bench command?

[ripienaar]

Got it. Thank you for the write-up! In, #1417, can I revert the auth change, so we can land --[no-]doubleack in nats bench command?

Does nats bench have sufficient test coverage to make that safe?

[alexbozhenko]

For that specific flag, it seems like there is no create/edit/IsSetByUser complexity.

And so I guess we either accept the fact that users who pass (ineffective) --no-doubleack will have their scripts broken, and will have to remove the flag, or we find some other non-breaking way to change this(e.g., by adding a default to help script, but I know you are against it.)

bench js consume [<flags>]
...
    --[no-]doubleack         Synchronously acknowledge messages, waiting for a
                             reply from the server

But I see how changing just that flag right now will not change the bigger picture.

[ripienaar]

Indeed that specific variable does appear safe to change, but this is the inherent problem, every single bool you want to change you have to open the code, look at the editor, find all the usages and actually study the code. For every boolean.

You cannot assume there are standard patters or consistency, this is a many years old project and some patterns emerged later than others and some code comes from external users who perhaps didnt follow any pattern.

The complexity of doing this is much, much more than any kind of "lets just change all" approach will allow.

There are things we can do to Fisk to make this better - but again, this is far more complex than naive attempts to just fix it for all booleans because booleans are used in many different situations like --help etc, actual care needs to be taken to consider all areas that this impacts as the surface area is very big. Care should also be taken that descriptions that users may have written for flags don't suddenly look like nonsense because perhaps we only show one of the variants of the flags.

I have a branch that does like this:

  --[no-]doubleack=false   Synchronously acknowledge messages, waiting for a reply from the serve

But not add redundant defaults like here

      --allow-batch                         Allow atomic batch publishing
      --allow-counter                       Configures the Stream as a distributed counter

etc, even that though - quite a big change and still I find edge cases by simply going through a large subset of commands and looking at how it behaves.

[ripienaar]

@alexbozhenko as you've related this affects many customers and is a regular things that comes up on customer calls lets focus spare time on this rather than super minor issues like #1427

To recap we want to change the --[no-]x flags to --no-x and not have --x anymore, we want to maintain a high degree of consistency and quality

This is a major engineering undertaking that:

• Changes a lot of user visible behaviour and removes flags users might call today - every --[no-]x has a --x and a --no-x which will now remove the --x variant with possible big impact
• Changes the meaning of a lot of the help output descriptions
• Changes a lot of variables, inverting some, affecting large portions of code
• Hard to do consistently which might mean splitting a lot of the add/edit paths up further than they are today for example Only show negation options for edit operations #1415

This is all high risk, but according to you very high priority and high impact for customer support team

Towards that it would be great if you could follow the following action plan here rather than spend time on minor issues like #1427

• Go through all commands and open sub issues to this one per command of all the flags in each command you wish to change
• Briefly sketch the desired --help outcome you want to achieve for that command
• Briefly mention major behaviours that will change while removing these flags
• Briefly mention major code changes required to deal with variable renamings, inverted behaviours etc, do this for each flag impacted

Once we agreed on the approach per command lets:

• Ensure test coverage captures all current use cases, contribute tests per command where they are lacking
• Perform the change on a specific command in its entirety as a single PR so the commands remain consistent

Again as you point out this issue is by your estimation cause of regular time wasted on customer calls and confusion for those customers so I'd appreciate if you focus your time that you wish to dedicate to CLI here rather than issues like #1427

[alexbozhenko]

Ok, I see with this thorough approach, it is quite a big undertaking.
Do we want all subissues first before submitting any PRs, or is it ok to tackle them one by one, and start with, e.g. simple ones, like audit gather ?

[ripienaar]

Yes I think consistency is key here - currently the behaviour is not optimal but it's consistent, so getting these things all implemented around the same time is best. So early steps first.

I am, still, of the opinion this is a very high risk for low reward thing and we should not do this.