Skip to content

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

Merged
fdncred merged 4 commits intonushell:mainfrom
WindSoilder:fix_if
Mar 17, 2023
Merged

make else if generate helpful error when condition have an issue#8274
fdncred merged 4 commits intonushell:mainfrom
WindSoilder:fix_if

Conversation

@WindSoilder
Copy link
Copy Markdown
Contributor

@WindSoilder WindSoilder commented Mar 1, 2023

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 WindSoilder mentioned this pull request Mar 1, 2023
Closed
@WindSoilder
Copy link
Copy Markdown
Contributor Author

WindSoilder commented Mar 1, 2023 •
edited
Loading

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

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
Copy Markdown
Member

sholderbach commented Mar 4, 2023

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

@WindSoilder
Copy link
Copy Markdown
Contributor Author

WindSoilder commented Mar 5, 2023 •
edited
Loading

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

WindSoilder commented Mar 9, 2023

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

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 9, 2023 •
edited
Loading

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

fdncred commented Mar 16, 2023

Are we ready to break things with this? LOL

@WindSoilder
Copy link
Copy Markdown
Contributor Author

WindSoilder commented Mar 17, 2023 •
edited
Loading

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

Reviewers

No reviews

Assignees

No one assigned

Labels

status:wait-until-after-nushell-release

Projects

None yet

Milestone

No milestone

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

@WindSoilder @fdncred @sholderbach @rgwood