Skip to content
This repository has been archived by the owner on May 19, 2018. It is now read-only.

Rephrase "assigning/binding to rvalue" errors to include context (#119) #123

Merged
merged 4 commits into from
Sep 22, 2016

Conversation

motiz88
Copy link
Contributor

@motiz88 motiz88 commented Sep 12, 2016

This replaces all "Assigning to rvalue" and "Binding rvalue" errors with more detailed variants based on the example in #119.

The implementation is the most straightforward one I could come up with - I add an optional context argument to checkLVal, toAssignable and toAssignableList, a simple string which is interpolated into the final error message (if any).

In some cases I made decisions that make the messages more informative in my view - e.g. making ArrayPattern pass through any parent context, so errors look like "invalid left-hand side in assignment expression" rather than "invalid left-hand side in array destructuring pattern".

Notes

  1. The wording is of course not set in stone and I'll happily implement any suggested changes.
  2. It's may be preferred aesthetically to move contextDescription leftwards in the methods' parameter lists, to avoid all those added undefineds. If there's a specific signature you'd like to see here - please let me know.

@codecov-io
Copy link

codecov-io commented Sep 12, 2016

Current coverage is 94.46% (diff: 100%)

Merging #123 into master will increase coverage by 0.04%

@@             master       #123   diff @@
==========================================
  Files            19         19          
  Lines          3105       3110     +5   
  Methods         327        327          
  Messages          0          0          
  Branches        816        817     +1   
==========================================
+ Hits           2932       2938     +6   
  Misses           94         94          
+ Partials         79         78     -1   

Powered by Codecov. Last update 650e333...85b22d0

@hzoo
Copy link
Member

hzoo commented Sep 22, 2016

I think it looks good overall? there's a merge conflict atm

traceur does: Invalid left-hand side expression in assignment: func() - dono if it's more helpful to add the left hand side in the error message?

What are the possible left hand values (Identifier/MemberExpression)? - maybe we can suggest that

@motiz88
Copy link
Contributor Author

motiz88 commented Sep 22, 2016

Working on that merge conflict now.

@motiz88
Copy link
Contributor Author

motiz88 commented Sep 22, 2016

FWIW, I'm personally not crazy about the idea of adding bits of code to error messages, for a number of reasons (though it's probably a matter of taste more than anything).

  • We wouldn't always want to clutter the message with the full left-hand code (imagine the output for func ( /* lots of arguments and complex expressions here... */ ) = 5), so we might introduce formatting and abbreviating logic, which can be nontrivial and lead to bloat.
  • There's already location information in the error object - my ideal workflow would be one where a tool / frontend just consumes and visualizes that (along with a clear, concise message), without babylon trying to hint at it separately via the error message.

@motiz88 motiz88 force-pushed the improve-rvalue-error-messages branch from 72cc7ee to 85b22d0 Compare September 22, 2016 14:39
@hzoo
Copy link
Member

hzoo commented Sep 22, 2016

We wouldn't always want to clutter the message

fair enough

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants