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

Better errors when bash-like operators are used #7241

Merged
merged 2 commits into from
Dec 7, 2022

Conversation

sophiajt
Copy link
Contributor

Description

Adds improved errors for when a user uses a bashism that nu doesn't support.

fixes #7237

Examples:

Error: nu::parser::shell_andand (link)

  × The '&&' operator is not supported in Nushell
   ╭─[entry #1:1:1]
 1 │ ls && ls
   ·    ─┬
   ·     ╰── instead of '&&', use ';' or 'and'
   ╰────
  help: use ';' instead of the shell '&&', or 'and' instead of the boolean '&&'
Error: nu::parser::shell_oror (link)

  × The '||' operator is not supported in Nushell
   ╭─[entry #8:1:1]
 1 │ ls || ls
   ·    ─┬
   ·     ╰── instead of '||', use 'try' or 'or'
   ╰────
  help: use 'try' instead of the shell '||', or 'or' instead of the boolean '||'
Error: nu::parser::shell_err (link)

  × The '2>' shell operation is 'err>' in Nushell.
   ╭─[entry #9:1:1]
 1 │ foo 2> bar.txt
   ·     ─┬
   ·      ╰── use 'err>' instead of '2>' in Nushell
   ╰────
Error: nu::parser::shell_outerr (link)

  × The '2>&1' shell operation is 'out+err>' in Nushell.
   ╭─[entry #10:1:1]
 1 │ foo 2>&1 bar.txt
   ·     ──┬─
   ·       ╰── use 'out+err>' instead of '2>&1' in Nushell
   ╰────
  help: Nushell redirection will write all of stdout before stderr.

User-Facing Changes

BREAKING CHANGES

This removes the && and || operators. We previously supported by &&/and and ||/or. With this change, only and and or are valid boolean operators.

Tests + Formatting

Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

  • cargo fmt --all -- --check to check standard code formatting (cargo fmt --all applies these changes)
  • cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style
  • cargo test --workspace to check that all tests pass

After Submitting

If your PR had any user-facing changes, update the documentation after the PR is merged, if necessary. This will help us keep the docs up to date.

@sholderbach sholderbach added the pr:breaking-change This PR implies a change affecting users and has to be noted in the release notes label Nov 25, 2022
#[diagnostic(
code(nu::parser::shell_outerr),
url(docsrs),
help("Nushell redirection will write all of stdout before stderr.")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe subject to change given #7240 ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, #7240 will make things different

@sophiajt sophiajt merged commit 379e3d7 into nushell:main Dec 7, 2022
@sophiajt sophiajt deleted the better_bashism_errors branch December 7, 2022 23:02
sholderbach added a commit to sholderbach/nushell that referenced this pull request Dec 21, 2022
`proptest` caught a failing test condition for `&&` as a literal string.

https://github.com/nushell/nushell/actions/runs/3753242377/jobs/6376308675

The change in the parser that now returns an error was introduced by
nushell#7241

This in theory doesn't have to be an error but it is probably better
safe than sorry to require quotation here.
sholderbach added a commit to sholderbach/nushell that referenced this pull request Dec 21, 2022
Requires a quotation to be parsed by current `from nuon`

Only required since nushell#7241

Include a comment stating that this is due to a zealous diagnostic
@sholderbach
Copy link
Member

Some of it is messing with quote-less nuon but should probably quoted anyways.

sholderbach added a commit that referenced this pull request Dec 21, 2022
`proptest` caught a failing test condition for `&&` as a literal string. It requires a quotation to be parsed correctly by current `from nuon`
    
https://github.com/nushell/nushell/actions/runs/3753242377/jobs/6376308675

The change in the parser that now returns an error was introduced by #7241

This in theory doesn't have to be an error (it is a diagnostic for nushell code) but it is probably better safe than sorry to require quotation here.

- Add a test for `&&` in `to nuon` from proptest fail
- Fix `to nuon` generating invalid `&&` literal
- Add a test for `,` in `to nuon`/`from nuon` cycle
  - Bonus: should already be properly quoted
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:breaking-change This PR implies a change affecting users and has to be noted in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add alternative sugguestion to error for &&
3 participants