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

Fix gobbleGroup not working with sequence expressions, i.e. "(1, 2)" #139

Merged
merged 3 commits into from
Apr 14, 2021

Conversation

6utt3rfly
Copy link
Collaborator

  • make the expressions[] parsing a separate function so it can be reused
  • instead of parsing a single expression, let it parse multiple expressions
  • Add test to compare against eprisma, which uses 'SequenceExpression' instead of 'Compound' in that case

Fixes #92

(it also allows for future arrow-function support, versus the code/comment from this PR )

- make the expressions[] parsing a separate function so it could be reused
- instead of parsing a single expression, let it parse multiple expressions
- Added test to compare against eprisma, which uses 'SequenceExpression' instead of 'Compound' in that case
@LeaVerou
Copy link
Collaborator

Hey, thanks! Are there any existing expressions that would be parsed differently now or does this only enable parsing for expressions that would previously throw?

…y array SEQUENCE_EXP) to match previous behavior

- added a couple more test cases against esprima
@6utt3rfly
Copy link
Collaborator Author

6utt3rfly commented Apr 14, 2021

Hey, thanks! Are there any existing expressions that would be parsed differently now or does this only enable parsing for expressions that would previously throw?

I pushed another commit to add a check for !nodes.length to return false so it matches previous behavior. So now the only difference should be that it will return a SequenceExpression node instead of throwing an Unclosed ( error.

The gobbleGroup code only gets run when the parser sees an open parentheses with no preceding identifier token (otherwise it would call gobbleArguments). So things like: (gobbleGroup) or identifer[1, (gobbleGroup)] or identifier((gobbleGroup)), etc. It also handles nested () properly now too: (((1))). I tried coming up with other expressions and comparing against the previous behavior but couldn't see any change.

@LeaVerou
Copy link
Collaborator

Thank you.
What about things like (1 + 2)? Is that still parsed the same?

@6utt3rfly
Copy link
Collaborator Author

You bet! Old/PR both respond the same, with a single node:

{
  "type": "BinaryExpression",
  "operator": "+",
  "left": {
    "type": "Literal",
    "value": 1,
    "raw": "1"
  },
  "right": {
    "type": "Literal",
    "value": 2,
    "raw": "2"
  }
}

@LeaVerou
Copy link
Collaborator

LGTM. Merging. Thanks again for working on this!

@LeaVerou LeaVerou merged commit c81912b into EricSmekens:master Apr 14, 2021
@6utt3rfly 6utt3rfly deleted the fix-gobble-group branch August 18, 2021 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(1, 1) produces error: Unclosed ( at character 6
2 participants