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

feat(parser): accept boolean literal with env vars #2664

Merged
merged 4 commits into from
Aug 10, 2021

Conversation

rami3l
Copy link
Contributor

@rami3l rami3l commented Aug 5, 2021

Closes #2539.

This PR allows boolean literals following the Python strtobool convention (case-insensitive):

True values are y, yes, t, true, on and 1; false values are n, no, f, false, off and 0.

  • A false literal or an absent env var will cause the flag to be considered absent.
  • If the env var gets a true literal, then the flag is considered present by the parser.
  • If the boolean literal is anything else, the parser returns an error.
  • If the env var gets anything else, then the flag is considered present by the parser (suggested by @pksunkara).

Questions

  • Should we adjust the APIs so that other conventions are also allowed?
  • How will this adjustment interact with the rest of the APIs?

Copy link
Member

@pksunkara pksunkara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be behind a new arg setting

@rami3l
Copy link
Contributor Author

rami3l commented Aug 6, 2021

@pksunkara Do you mean enabling the new behavior with a flag?

I understand that making the new behavior as the default will be a breaking change.

However, the main issue that I personally want to address is that, ENV_VAR=false is currently considered as true. It seems to be a highly possible misuse of the current API.

By keeping the old behavior by default, the misuse will probably continue. What do you think could be the appropriate way of handling this?

@pksunkara
Copy link
Member

You are right. But I think any other random value should still be reduced to true instead of raising error. What do you think?

@rami3l
Copy link
Contributor Author

rami3l commented Aug 9, 2021

@pksunkara Thanks for your advice!

I assume that one can still use something like the pseudo-flag pattern if they do want an error. In that case, what you said could be a better default.

Sorry, I'm a bit occupied right now. I will catch up with the refactoring a bit later :)

@pksunkara
Copy link
Member

Yup. Sounds good. Will wait for an update then.

@rami3l
Copy link
Contributor Author

rami3l commented Aug 10, 2021

@pksunkara Rebased. Also replaced nested braces with early returns for readability.

Suggestion: Since the pseudo-flag pattern seems to play such an important role in both env flags and CLI flags, maybe we should try to improve its visibility.

@rami3l rami3l marked this pull request as ready for review August 10, 2021 10:46
src/parse/parser.rs Outdated Show resolved Hide resolved
src/build/arg/mod.rs Outdated Show resolved Hide resolved
@pksunkara pksunkara merged commit 69d7594 into clap-rs:master Aug 10, 2021
@rami3l rami3l deleted the env-literal branch August 10, 2021 23:13
bisgardo added a commit to bisgardo/concordium-docker that referenced this pull request Mar 22, 2022
Because of the way that the node interprets boolean env vars (through 'clap'), defining the custom var `TXLOG_ENABLED` actually left no way of *not* enabling tx logging. The reason is that setting `CONCORDIUM_NODE_TRANSACTION_OUTCOME_LOGGING=${TXLOG_ENABLED}` defined the variable (with empty value) even when `TXLOG_ENABLED` wasn't defined. Until 'clap' v3.0.0 (which the node hasn't been upgraded to), this resolves the variable to `true` (see 'clap-rs/clap#2664').

Once the node has upgraded to a new version of 'clap', we can re-introduce `TXLOG_ENABLED` with default value `false` and have it work as expected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change behavior of flag arguments specified as env vars
2 participants