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

New function alias: env() #1613

Merged
merged 10 commits into from
Jun 13, 2023
Merged

New function alias: env() #1613

merged 10 commits into from
Jun 13, 2023

Conversation

kykyi
Copy link
Contributor

@kykyi kykyi commented Jun 7, 2023

In relation to #1278, this PR adds a new env function which acts as an alias to both env_var and env_var_or_default:

env_var('x') => env('x')
env_var_or_default('x', 42) => env('x', 42)

To make such a function signature possible, a new UnaryOpt enum variant was added which accounts for the majority of the touched files!

@kykyi kykyi changed the title Enhancement/env alias New function alias: env() Jun 7, 2023
@casey
Copy link
Owner

casey commented Jun 12, 2023

Thanks for the PR! It looks like the env function will silently ignore excess arguments, which isn't great. How about something like:

  UnaryOpt(fn(&FunctionContext, &str, Option<&str>) -> Result<String, String>),

Which accepts only one or two arguments?

@kykyi
Copy link
Contributor Author

kykyi commented Jun 13, 2023

Good call @casey! I have updated to introduce UnaryOpt 👌

@@ -2334,6 +2334,34 @@ mod tests {
},
}

error! {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests break the function_argument_<variant> convention by adding a too-many and a too-few test, unsure if this is what you want or should just have the single test.

src/thunk.rs Outdated
Comment on lines 71 to 75
let a = Box::new(arguments.remove(0));
let opt_b = match arguments.pop() {
Some(value) => Box::new(Some(value)),
None => Box::new(None),
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove(0) to grab from the front of the list, then a pop on the last element, happy to hear a prettier way 😅

@casey casey enabled auto-merge (squash) June 13, 2023 12:47
@casey casey merged commit bba673f into casey:master Jun 13, 2023
@casey
Copy link
Owner

casey commented Jun 13, 2023

Nice, merged!

@kykyi kykyi deleted the enhancement/env-alias branch June 13, 2023 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants