Skip to content

updates let-env signature to remove required params#9917

Merged
fdncred merged 2 commits intonushell:mainfrom
fdncred:update_let_env_signature
Aug 4, 2023
Merged

updates let-env signature to remove required params#9917
fdncred merged 2 commits intonushell:mainfrom
fdncred:update_let_env_signature

Conversation

@fdncred
Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
Contributor

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
Copy Markdown
Contributor 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
@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.

3 participants