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

parsing of PC_DISABLE_TUI should be consistent with -t flag #75

Closed
adrian-gierakowski opened this issue Jul 20, 2023 · 5 comments
Closed
Labels
done Done, awaiting release

Comments

@adrian-gierakowski
Copy link
Contributor

adrian-gierakowski commented Jul 20, 2023

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 adrian-gierakowski changed the title setting PC_DISABLE_TUI env vart to empty string should be parsed as false setting PC_DISABLE_TUI env var to empty string should be parsed as false Jul 20, 2023
@adrian-gierakowski adrian-gierakowski changed the title setting PC_DISABLE_TUI env var to empty string should be parsed as false handling of PC_DISABLE_TUI should be more consistent Jul 20, 2023
@adrian-gierakowski adrian-gierakowski changed the title handling of PC_DISABLE_TUI should be more consistent parsing of PC_DISABLE_TUI should be consistent with -t flag Jul 20, 2023
@adrian-gierakowski
Copy link
Contributor Author

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
Copy link
Contributor Author

@F1bonacc1 what do you think about the above?

@F1bonacc1
Copy link
Owner

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
Copy link
Contributor Author

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 F1bonacc1 added done Done, awaiting release and removed need more info labels May 6, 2024
@F1bonacc1
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
done Done, awaiting release
Projects
None yet
Development

No branches or pull requests

2 participants