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

Allow recipe parameters to shadow variables #1480

Merged
merged 3 commits into from Jan 10, 2023

Conversation

casey
Copy link
Owner

@casey casey commented Jan 6, 2023

Recipe parameters cannot have the same name as bindings in the environment.

This has been striking me as an unnecessarily conservative restriction, and counter to the way that most programming languages work, so this PR removes it.

I'm interested in feedback before merging.

@neunenak Thoughts?

name: shadowing_parameters_do_not_change_environment,
justfile: "export FOO := 'hello'\na FOO:\n echo $FOO\n",
args: ("a", "bar"),
stdout: "hello\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not immediately clear to me why passing "bar" on the command line doesn't override "hello" here, but it does in the other two cases.

Copy link
Owner Author

Choose a reason for hiding this comment

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

In this case, $FOO refers to an environment variable, which comes from the outer export. In the first test, FOO refers to the just variable, not the environment variable. In the last case, the just parameter FOO is exported with $FOO, so it now refers to the exported parameter.

@neunenak
Copy link
Contributor

neunenak commented Jan 6, 2023

I don't see any issues. In fact, I think this might've made a justfile that I used at my previous job that was trying to provide a default env var value in the justfile but also allow it to be overridden on the command line to be slightly more concise.

@casey
Copy link
Owner Author

casey commented Jan 6, 2023

Thanks for the feedback! I'm going to let this marinate for a week and then probably merge it.

@casey casey enabled auto-merge (squash) January 10, 2023 00:56
@casey casey merged commit af54daf into master Jan 10, 2023
@casey casey deleted the allow-parameters-to-shadow-variables branch January 10, 2023 00:59
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.

None yet

2 participants