boolean or string boolean, that's the question

[martinlingstuyl]

Hi people,

Just the other day I was using an spo commandlet (from PowerShell) to set some field on a content type and I tried to use the hidden option with the PowerShell value for true: $true...

m365 spo contenttype field set --webUrl '<some-url>' --contentTypeId '<some-contenttype-id>' --fieldId '<some-field-id>' --hidden $true

Now that does not work, I needed to use the string value 'true' or 'false'. What's happening here in the code base is that the option hidden has been created as a string type, instead of as a boolean type.

It turns out that quite a few commands have this situation, they even have autocomplete values.
But it's not consistent across the code base. Sometimes we use boolean and sometimes string.

I think using strings for booleans is unfortunate, because I'm always just using booleans in my script, for example from variables that have been assigned from the output of other functions. I'm now having to write extra scripting to take care of feeding the right string into the option.

And also, it's inconsistent... I think we need to pick sides here. We either do things with booleans OR with strings.
I'm hugely pro-boolean of course. 😁

Updating this would be a rather big refactoring and a huge breaking change. I'm also thinking it's too late for v6.

What are your ideas on this. I'd really like to hear it!

[waldekmastykarz]

Just to check if I understand correctly: do you mean the use of --flag vs --option true|false or do you mean how we type the option's value in the command's TypeScript code?

[martinlingstuyl]

I don't mean the flag. It's the combination of the typing, autocomplete and validation. It expects 'true', not true.

[waldekmastykarz]

In bash, everything is a string, which is why we're expecting 'true' instead of true. We'd need to check what's minimist doing for us automatically and if we shouldn't list options that expect a boolean value as boolean type explicitly and treat them as booleans. The advantage of treating them as strings is that we can perform validations and return a meaningful error rather than expecting minimist to perform validation and assume a certain value when the provided value isn't valid. That said, the approach we use should be consistent.

[martinlingstuyl]

The advantage of treating them as strings is that we can perform validations and return a meaningful error rather than expecting minimist to perform validation and assume a certain value when the provided value isn't valid

That's a good point.

We currently have minimist configured to convert to Booleans if they are so typed. But according to a comment in the minimist typings that only flies for double dashed options. If people use -o true or --option=true, it will not work the same... (i still need to check this out myself)

So that appears to be not consistent. I'm not a fan of inconsistency. Also we got an issue where minimist converts empty option values to booleans. (#3718) this might be related.

Seeing that together we might choose the route of moving everything to string typings and disabling minimist Boolean conversion. It will take some extra work, but it should be possible to fix PoSH $true/$false, so that we can use both 'true','false' as well as $true/$false. We can leave the autocompleter in, and add validations everywhere.

I'll do some extra research when I find the time.

[waldekmastykarz]

Seeing that together we might choose the route of moving everything to string typings and disabling minimist Boolean conversion

Let's be mindful of introducing breaking changes, that we shouldn't just do and instead should announce well ahead of the time.

[garrytrinder]

I personally think that using options, e.g. --hidden <true|false> looks odd in a CLI. I can't think of an example where I would use this in others such as Azure CLI or git, options should accept strings in my mind.

My preference would be to use flags for booleans, e.g. --hide and --show for true and false values, would be a better approach, this would work across different shell, is more readable and avoids the string vs boolean issue.

One downside to using flags though is that it makes scripting a little more difficult as you have to construct the command options dynamically rather than just changing the option value, I expect this is the reason behind this inconsistency that you have discovered.

[waldekmastykarz]

If you agree with my proposal of using az cli's definition, then PowerShells $true and $false are covered as well. As the definition states that booleans are case-insensitive, and $true/$false will translate to "True"/"False".

If PowerShell's $true show up as True|1|yes|true|on in our code, then that's sorted: it doesn't require us to do anything shell-specific and smoothens an edge for folks who use PowerShell and instinctively use $true|$false.

[martinlingstuyl]

I'll do some extra checks on what minimist does.

To think ahead: we would need some standard validation logic (util function) and a util function to get the actual bool value from the string value if necessary. Is that also what you're thinking?

[waldekmastykarz]

We already have a boolean validation function here:

cli-microsoft365/src/utils/validation.ts

Line 107 in 005c47a

isValidBoolean(value: string): boolean {

, so we'd simply extend it with the additional values that we allow. We'd need to add another function like parseBoolean that takes the string value specified by the user and returns a bool. That one would be more suitable in formatting I think @ https://github.com/pnp/cli-microsoft365/blob/main/src/utils/formatting.ts.

[martinlingstuyl]

I think it's time we move this to issues. Will do that soon!

[martinlingstuyl]

Before we start implementing this, let's also check if minimist doesn't already support 1|yes|true|on as valid values for a bool(true)

This appears not to be the case. the value off, 0 and False translate to true when minimist parses them. Even if they option is added to the booleans type list.

Moving this to issues for fixing..

[martinlingstuyl]

Moved this to issues here: #3853