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

updates let-env signature to remove required params #9917

Merged
merged 2 commits into from Aug 4, 2023

Conversation

fdncred
Copy link
Collaborator

@fdncred fdncred commented Aug 4, 2023

Description

This PR changes the signature of the deprecated command let-env so that it does not mislead people when invoking it without parameters.

Before

> let-env
Error: nu::parser::missing_positional

  × Missing required positional argument.
   ╭─[entry #2:1:1]
 1  let-env
   ╰────
  help: Usage: let-env <var_name> = <initial_value>

After

 let-env
Error: nu::shell::deprecated_command

  × Deprecated command let-env
   ╭─[entry #1:1:1]
 1  let-env
   · ───┬───
   ·    ╰── 'let-env' is deprecated. Please use '$env.<environment variable> = ...' instead.
   ╰────

User-Facing Changes

Tests + Formatting

After Submitting

Copy link
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

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

but now we get

> let-env FOO = "foo"
Error: nu::parser::extra_positional

  × Extra positional argument.
   ╭─[entry #1:1:1]
 1  let-env FOO = "foo"
   ·         ─┬─
   ·          ╰── extra positional argument
   ╰────
  help: Usage: let-env

instead of

> let-env FOO = "foo"
Error: nu::shell::deprecated_command

  × Deprecated command let-env
   ╭─[entry #18:1:1]
 1  let-env FOO = "foo"
   · ───┬───
   ·    ╰── 'let-env' is deprecated. Please use '$env.<environment variable> = ...' instead.
   ╰────

🤔

@fdncred
Copy link
Collaborator Author

fdncred commented Aug 4, 2023

Apparently, we have to "pick our poison". At least it's not telling you to use an old syntax and when you're typing let-env FOO = blah everything after let-env is red, at least in my theme, indicating it's invalid. I think I'd land this PR because it's better than the previous error.

@sophiajt
Copy link
Member

sophiajt commented Aug 4, 2023

As mentioned in discord, I believe this will work if you make the params optional rather than required (or removed).

@fdncred
Copy link
Collaborator Author

fdncred commented Aug 4, 2023

updated to optional, seems to be working as expected now.

@fdncred fdncred merged commit 2b431f9 into nushell:main Aug 4, 2023
19 checks passed
@fdncred fdncred deleted the update_let_env_signature branch August 4, 2023 19:07
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

3 participants