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

make else if generate helpful error when condition have an issue #8274

Merged
merged 4 commits into from
Mar 17, 2023

Conversation

WindSoilder
Copy link
Collaborator

Description

Fixes: #7575

User-Facing Changes

Previously:

if❯ if false { "aaa" } else if $a { 'a' }
Error: nu::parser::parse_mismatch

  × Parse mismatch during operation.
   ╭─[entry #10:1:1]
 1 │ if false { "aaa" } else if $a { 'a' }
   ·                         ─┬
   ·                          ╰── expected block, closure or record
   ╰────

After:

❯ if false { "aaa" } else if $a { 'a' }
Error: nu::parser::variable_not_found

  × Variable not found.
   ╭─[entry #1:1:1]
 1 │ if false { "aaa" } else if $a { 'a' }
   ·                            ─┬
   ·                             ╰── variable not found
   ╰────

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.

@WindSoilder
Copy link
Collaborator Author

WindSoilder commented Mar 1, 2023

It breaks the following mut usage:

stderr: Error: nu::parser::expected_keyword

  × Capture of mutable variable.
   ╭─[/tmp/.tmpaaj31c:1:1]
 1 │ mut x = 100; if 2 > 3 { $x = 200 } else { $x = 300 }; $x 
   ·                                           ─┬
   ·                                            ╰── capture of mutable variable
   ╰────

Because { $x = 300 } is parsed as a closure, and it doesn't allowed to capture mutable variable, a solution would be restrict closure syntax to not mess up with block syntax

@fdncred
Copy link
Collaborator

fdncred commented Mar 1, 2023

Because { $x = 300 } is parsed as a closure, and it doesn't allowed to capture mutable variable, a solution would be restrict closure syntax to not mess up with block syntax

ahhhh, that makes sense now. I've been having problems with else if for weeks now. We definitely need to fix that.

@sholderbach
Copy link
Member

How can we resolve the remaining issues here? Do we need the block/closure separation to land?

@WindSoilder
Copy link
Collaborator Author

WindSoilder commented Mar 5, 2023

Yeah, #8290 needs to be mreged first(block by pr on virtualenv..), or else I have to try another solution, I'd prefer to merge #8290 first(when it's ok)

@WindSoilder WindSoilder marked this pull request as ready for review March 9, 2023 09:26
@WindSoilder
Copy link
Collaborator Author

I've re-implement it and the implementation doesn't rely on #8273

@codecov
Copy link

codecov bot commented Mar 9, 2023

Codecov Report

Merging #8274 (b882f47) into main (03e688e) will increase coverage by 0.01%.
The diff coverage is 70.58%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8274      +/-   ##
==========================================
+ Coverage   68.49%   68.51%   +0.01%     
==========================================
  Files         620      620              
  Lines       99472    99488      +16     
==========================================
+ Hits        68133    68160      +27     
+ Misses      31339    31328      -11     
Impacted Files Coverage Δ
crates/nu-parser/src/parser.rs 82.52% <70.58%> (+0.03%) ⬆️
crates/nu-cli/src/util.rs 67.47% <0.00%> (-1.04%) ⬇️
crates/nu-protocol/src/ty.rs 87.14% <0.00%> (+0.71%) ⬆️
src/config_files.rs 25.88% <0.00%> (+1.17%) ⬆️
crates/nu-parser/src/lex.rs 91.39% <0.00%> (+3.56%) ⬆️

@fdncred
Copy link
Collaborator

fdncred commented Mar 16, 2023

Are we ready to break things with this? LOL

@WindSoilder
Copy link
Collaborator Author

WindSoilder commented Mar 17, 2023

Yeah, I think we can land it

@fdncred fdncred merged commit eb2e2e6 into nushell:main Mar 17, 2023
@WindSoilder WindSoilder deleted the fix_if branch August 2, 2023 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

else if causes unhelpful parsing error in 0.73.0 when condition has an issue
4 participants