Conversation
pytrajplot/main.py
Outdated
| @click.option( | ||
| "--skip-ssm-fallback", | ||
| is_flag=True, | ||
| default=False, |
There was a problem hiding this comment.
I think I would default to True here so that the implementation at CSCS would not need to change its args.
There was a problem hiding this comment.
As --skip-ssm-fallback is a flag, defaulting it to true might not be practical because then setting it without a value would be the same as not setting it? Would it be better to invert the logic to a --use-ssm-fallback flag so that setting it without a value has an effect? Alternatively, could the --ssm-parameter-path be used for this functionality and the fallback is skipped if --ssm-parameter-path is not present? Unless there is an advantage to keep the --ssm-parameter-path even when the fallback should be skipped.
There was a problem hiding this comment.
True, sorry i misinterpreted the flag. Yes I would be fine to leave as is, True = skip. But I think I prefer the suggestion that if --ssm-parameter-path is set to True or if SSM_PARAMETER_PATH env var is defined, i would run the fallback, and otherwise skip.
pirmink
left a comment
pirmink
left a comment
There was a problem hiding this comment.
Looks good, while I support @victoria-cherkas suggestions.
pytrajplot/main.py
Outdated
| @click.option( | ||
| "--skip-ssm-fallback", | ||
| is_flag=True, | ||
| default=False, |
There was a problem hiding this comment.
As --skip-ssm-fallback is a flag, defaulting it to true might not be practical because then setting it without a value would be the same as not setting it? Would it be better to invert the logic to a --use-ssm-fallback flag so that setting it without a value has an effect? Alternatively, could the --ssm-parameter-path be used for this functionality and the fallback is skipped if --ssm-parameter-path is not present? Unless there is an advantage to keep the --ssm-parameter-path even when the fallback should be skipped.
3 participants
No description provided.