feat: Improved variable handling for config #332
Conversation
This allows an environment variable in any position for headers as long
as it is in format `$VAR` or `${VAR}` allowing alphanumeric characters
and underscore `_`. This is now also applied to `remote_schema_url` in
addition to headers.
Fixes mirumee#328
Relates to mirumee#231
|
|
||
|
|
||
| def _replace_env_vars(value: str) -> str: | ||
| pattern = re.compile(r"\${?([\w_]+)}?") |
There was a problem hiding this comment.
I didn't add support for ${} because I think it's confusing to have it without proper matching. Try $TEST_VAR} or ${TEST_VAR. Also consider variables like $1WOOT. I think of simple expressions, mine (\$([A-z][A-z0-9_]*)) is a bit more optimal.
There was a problem hiding this comment.
Environment variables tend to come from a shell such as bash or zsh where ${VAR} is a common usage and even required if you want a variable like ${foo}bar which wouldn't work with your suggestion.
I didn't do any complex matching to ease maintenance, using something like $TEST_VAR} would just break your own code and I don't know why one would do that. I guess worst case we hide a configuration issue since it would successfully replace it if $TEST_VAR is defined.
If we want to actually check for this I think just adding two options would be easiest, something like, \${([\w_]+)}|\$([\w_]+) would require either ${VAR} or $VAR.
Also consider variables like
$1WOOT.
What should be considered you mean? I know it's not a valid variable in bash but do you mean if someone want's to use the literal we shouldn't replace it? Seems unlikely to me but can be fixed with something like \${([A-z][A-z0-9_]*)}|\$([A-z][A-z0-9_]*). It's a trade-off I guess that I thought wasn't worth it but I'm open to change it if the owners agree.
Previous owners didn't even want this feature in so we'll see if this will even be considered 😺
There was a problem hiding this comment.
Seems unlikely to me but can be fixed with something like
\${([A-z][A-z0-9_]*)}|\$([A-z][A-z0-9_]*). It's a trade-off I guess that I thought wasn't worth it but I'm open to change it if the owners agree.
Yes, this type of regex is what I was alluding to with both points. I'm afraid you can't use multiple match groups with your approach. See #349 for a suggestion if proper ${} support is desired.
Previous owners didn't even want this feature in so we'll see if this will even be considered 😺
As I said, I don't think it's a good idea to stick with a weird, non-standard notion of variables that confuses users and doesn't save on parsing complexity.
If they stick to it, it would at least be worth documenting to avoid head-banging.
/cc @mat-sop
This allows an environment variable in any position for headers as long as it is in format
$VARor${VAR}allowing alphanumeric characters and underscore_. This is now also applied toremote_schema_urlin addition to headers.Fixes #328
Relates to #231
I don't know if this is desired given what was said in #231 but the fix felt easy enough and I don't see much risk of pulling in
resince it's a one time pass of the config which would be minimal compared to the rest where time is spent. Also didn't feel it added much more complexity.