feat(parser): accept boolean literal with env vars#2664
feat(parser): accept boolean literal with env vars#2664pksunkara merged 4 commits intoclap-rs:masterfrom
Conversation
pksunkara
left a comment
There was a problem hiding this comment.
This needs to be behind a new arg setting
|
@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, By keeping the old behavior by default, the misuse will probably continue. What do you think could be the appropriate way of handling this? |
|
You are right. But I think any other random value should still be reduced to |
|
@pksunkara Thanks for your advice! I assume that one can still use something like the Sorry, I'm a bit occupied right now. I will catch up with the refactoring a bit later :) |
|
Yup. Sounds good. Will wait for an update then. |
|
@pksunkara Rebased. Also replaced nested braces with early returns for readability. Suggestion: Since the |
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.
Closes #2539.
This PR allows boolean literals following the Python
strtoboolconvention (case-insensitive):falseliteral or an absent env var will cause the flag to be considered absent.If the env var gets atrueliteral, then the flag is considered present by the parser.If the boolean literal is anything else, the parser returns an error.Questions
pseudo-flagpattern should be used in this case.