F1bonacc1/process-compose

parsing of PC_DISABLE_TUI should be consistent with -t flag

Closed this issue · 5 comments

adrian-gierakowski commented

Defect

Currently all of the following will result in TUI being disabled:

PC_DISABLE_TUI=false process-compose
PC_DISABLE_TUI=true process-compose
PC_DISABLE_TUI=1 process-compose
PC_DISABLE_TUI=0 process-compose
PC_DISABLE_TUI=whatever process-compose
PC_DISABLE_TUI= process-compose

So the only way override PC_DISABLE_TUI once it's set is by un-setting the env var, or by adding -t flag.

By comparison, when using t flag following result in tui being disabled:

process-compose -t=0
process-compose -t=false

while following results in it being enabled:

process-compose -t=1
process-compose -t=true
process-compose -t

anything else (like t=whatever) is an error

Version of process-compose:

Version:        v0.51.4
Commit:         e3cc52e
Date (UTC):     2023-06-16T18:14:56Z

OS environment:

nixos

Steps or code to reproduce the issue:

runt

Expected result:

I would expect the parsing of PC_DISABLE_TUI to be consistent with parsing o -t

Btw. I think it's a bit confusing that PC_DISABLE_TUI is reverse of -t. I would deprecate PC_DISABLE_TUI and add PC_TUI instead.

Actual result:

setting PC_DISABLE_TUI to any value is interpreted as true

adrian-gierakowski commented

one use case is to address a comment here

also, if PC_DISABLE_TUI=false worked as expected then in the linked code above, we could do

  ''export PC_DISABLE_TUI=''${PC_DISABLE_TUI-${builtins.toJSON (!config.tui)}}''

which would allow user to override the default value by setting PC_DISABLE_TUI before executing the wrapper

adrian-gierakowski commented

@F1bonacc1 what do you think about the above?

F1bonacc1 commented

Hey @adrian-gierakowski,

I always considered it standard practice to use any value in an env variable acting as a boolean.
Having said that, I see how this can be an issue in a wrapped situation.

I would prefer not to break the existing behavior and remain backward compatible.
Will it work if I change the behavior to be:

  • From: env var defined
  • To: env var defined and length > 0

This way you will be able to reset it with something like PC_DISABLE_TUI=""

adrian-gierakowski commented

I always considered it standard practice to use any value in an env variable acting as a boolean

Could you please elaborate, I am not sure what you mean.

I believe that if an env var is provided as and alternative to a cli flag, then following the principle of least surprise, one should be able to use the same values with it as with the flag to achieve the same result.

Your proposed solution is fine for me, but personally I’d change it to support the same values as the flag, if not now then at least in the next major release.

F1bonacc1 commented

Fixed in v1.5.0.
You can now set PC_DISABLE_TUI="" or PC_DISABLE_TUI="false"

  • ❤️1