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

More specific errors for missing values in records #11423

Merged
merged 3 commits into from
Dec 27, 2023

Conversation

ysthakur
Copy link
Member

Description

Currently, when writing a record, if you don't give the value for a field, the syntax error highlights the entire record instead of pinpointing the issue. Here's some examples:

> { a: 2, 3 } # Missing colon (and value)
Error: nu::parser::parse_mismatch

  × Parse mismatch during operation.
   ╭─[entry #2:1:1]
 1   { a: 2, 3 }
   ·  ─────┬─────
   ·       ╰── expected record
   ╰────

> { a: 2, 3: } # Missing value
Error: nu::parser::parse_mismatch

  × Parse mismatch during operation.
   ╭─[entry #3:1:1]
 1   { a: 2, 3: }
   ·  ──────┬─────
   ·        ╰── expected record
   ╰────

> { a: 2, 3 4 } # Missing colon
Error: nu::parser::parse_mismatch

  × Parse mismatch during operation.
   ╭─[entry #4:1:1]
 1   { a: 2, 3 4 }
   ·  ──────┬──────
   ·        ╰── expected record
   ╰────

In all of them, the entire record is highlighted red because an Expr::Garbage is returned covering that whole span:

image

This PR is for highlighting only the part inside the record that could not be parsed. If the record literal is big, an error message pointing to the start of where the parser thinks things went wrong should help people fix their code.

User-Facing Changes

Below are screenshots of the new errors:

If there's a stray record key right before the record ends, it highlights only that key and tells the user it expected a colon after it:

image

If the record ends before the value for the last field was given, it highlights the key and colon of that field and tells the user it expected a value after the colon:

image

If there are two consecutive expressions without a colon between them, it highlights everything from the second expression to the end of the record and tells the user it expected a colon. I was tempted to add a help message suggesting adding a colon in between, but that may not always be the right thing to do.

image

Tests + Formatting

After Submitting

@fdncred
Copy link
Collaborator

fdncred commented Dec 25, 2023

nice! this looks a lot better!

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.

this is much nicer and the changes are quite simple as well, nice job 👌

could we add some tests for this to make sure the errors stay great like this in the future? 😇

@ysthakur
Copy link
Member Author

@amtoine I only added tests to check if the error message contained expected ':' or expected value, not for checking highlighting of returned spans, but I assume we don't need to be that thorough anyway? I couldn't find any tests in src/tests or the parser crate's tests that check which specific span(s) the error highlighted

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.

@amtoine I only added tests to check if the error message contained expected ':' or expected value, not for checking highlighting of returned spans, but I assume we don't need to be that thorough anyway? I couldn't find any tests in src/tests or the parser crate's tests that check which specific span(s) the error highlighted

i think that's perfect, thanks for working on this improvement 🙏

@amtoine amtoine merged commit 9522052 into nushell:main Dec 27, 2023
19 checks passed
@ysthakur ysthakur deleted the invalid-record-highlight branch January 12, 2024 21:12
@kubouch kubouch added the pr:errors This PR improves our error messages label Jan 17, 2024
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this pull request Feb 20, 2024
# Description
Currently, when writing a record, if you don't give the value for a
field, the syntax error highlights the entire record instead of
pinpointing the issue. Here's some examples:

```nushell
> { a: 2, 3 } # Missing colon (and value)
Error: nu::parser::parse_mismatch

  × Parse mismatch during operation.
   ╭─[entry nushell#2:1:1]
 1 │  { a: 2, 3 }
   ·  ─────┬─────
   ·       ╰── expected record
   ╰────

> { a: 2, 3: } # Missing value
Error: nu::parser::parse_mismatch

  × Parse mismatch during operation.
   ╭─[entry nushell#3:1:1]
 1 │  { a: 2, 3: }
   ·  ──────┬─────
   ·        ╰── expected record
   ╰────

> { a: 2, 3 4 } # Missing colon
Error: nu::parser::parse_mismatch

  × Parse mismatch during operation.
   ╭─[entry nushell#4:1:1]
 1 │  { a: 2, 3 4 }
   ·  ──────┬──────
   ·        ╰── expected record
   ╰────
```

In all of them, the entire record is highlighted red because an
`Expr::Garbage` is returned covering that whole span:


![image](https://github.com/nushell/nushell/assets/45539777/36660b50-23be-4353-b180-3f84eff3c220)

This PR is for highlighting only the part inside the record that could
not be parsed. If the record literal is big, an error message pointing
to the start of where the parser thinks things went wrong should help
people fix their code.

# User-Facing Changes
Below are screenshots of the new errors:

If there's a stray record key right before the record ends, it
highlights only that key and tells the user it expected a colon after
it:


![image](https://github.com/nushell/nushell/assets/45539777/94503256-8ea2-47dd-b69a-4b520c66f7b6)

If the record ends before the value for the last field was given, it
highlights the key and colon of that field and tells the user it
expected a value after the colon:


![image](https://github.com/nushell/nushell/assets/45539777/2f3837ec-3b35-4b81-8c57-706f8056ac04)

If there are two consecutive expressions without a colon between them,
it highlights everything from the second expression to the end of the
record and tells the user it expected a colon. I was tempted to add a
help message suggesting adding a colon in between, but that may not
always be the right thing to do.


![image](https://github.com/nushell/nushell/assets/45539777/1abaaaa8-1896-4909-bbb7-9a38cece5250)

# Tests + Formatting

# After Submitting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:errors This PR improves our error messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants