Ablity to use own enviornment variables in config files #344
Conversation
|
Hello, just a few things before I review this PR.
Also, is there any reason to come up with your own regex over using a crate like envsubst? Please don't change your solution, I'm just curious to know your thoughts. |
|
Actually no, I didn't know about envsubst, could you tell me how to add unit tests?, I had tested using a toml file, it works well with it. |
|
Down the You can create a unit test for |
|
thanks✌️, I'll do that |
|
I have added test for only substitute_env_vars() alone not combining test_override_with_env, if required I'll add. |
|
made more changes:-
|
|
also please note:- in some places i have replaced std::env to just env |
| if !missing_vars.is_empty() { | ||
| return Err(format!( | ||
| "Missing environment variables: {}", | ||
| missing_vars.join(", ") | ||
| )); | ||
| } |
There was a problem hiding this comment.
I like that you have this to inform users about env vars missing.
| let input = r#" | ||
| [section1] | ||
| key1 = "${TEST_VAR1}" | ||
| key2 = "${TEST_VAR-default}" |
There was a problem hiding this comment.
For a future PR: we should ignore default variables or let the user know that we don't support it.
| let mut result = String::new(); | ||
|
|
||
| for line in content.lines() { | ||
| if !line.trim_start().starts_with('#') { |
There was a problem hiding this comment.
Interesting that you are ignoring comment lines.
There was a problem hiding this comment.
fortunately there was a variable in the comments of minimal_config, which failed the new test[✌️]
|
Thanks alot 🙌 |
Fixes #336
I have tested the function alone in a separate environment, please do test it with entire project.