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

fix(cheatcode): envOr use default for empty strings #9706

Closed
wants to merge 2 commits into from

Conversation

grandizzy
Copy link
Collaborator

Motivation

closes #7408

  • use default if string env var empty when vm.envOr(string)

Solution

@grandizzy grandizzy marked this pull request as ready for review January 17, 2025 15:42
@DaniPopes
Copy link
Member

Ah I see, honestly I don't agree with this, the empty string is obviously a valid string value to have. If you don't want it to be set at all then you don't set it, commenting it in .env.

@grandizzy
Copy link
Collaborator Author

Ah I see, honestly I don't agree with this, the empty string is obviously a valid string value to have. If you don't want it to be set at all then you don't set it, commenting it in .env.

Yeah, don't have a strong opinion, happy to close this PR and #7408 as working as designed if we're on the same page @yash-atreya @zerosnacks wdyt?

@@ -258,7 +258,10 @@ pub fn set_execution_context(context: ForgeContext) {
}

fn env(key: &str, ty: &DynSolType) -> Result {
get_env(key).and_then(|val| string::parse(&val, ty).map_err(map_env_err(key, &val)))
get_env(key).and_then(|val| {
ensure!(!val.is_empty(), "env var value cannot be empty");
Copy link
Member

Choose a reason for hiding this comment

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

an empty env var is valid.
I assume some users do this to erase the value, so some would treat "" as delete, but I think we shouldn't do this and force them to properly unset envvars instead?

@DaniPopes
Copy link
Member

Closing based on above.

@DaniPopes DaniPopes closed this Jan 18, 2025
@grandizzy grandizzy deleted the issue-7408 branch January 18, 2025 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

bug(cheatcodes): differentiate between FOO= (unset) and FOO="" (empty) in vm.envOr
3 participants