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: tokenize the latest right curly brace (fixes #403) #419

Merged
merged 2 commits into from
Aug 15, 2019

Conversation

finico
Copy link
Contributor

@finico finico commented Jul 30, 2019

There is a logic in TokenTranslator::onToken about the latest curly brace
https://github.com/eslint/espree/blob/6ebc21947166399a0b4918d4a1beb9d610650336/lib/token-translator.js#L196-L204
But it executes only at first eof token (it seems strange that the first token is eof, but it comes from Acorn's init state)
I've added a call of next for the TokenTranslator::onToken to be executed with the last eof token

And I've noticed two failed tests, but they've already fixed at #416

@kaicataldo
Copy link
Member

Hi, thanks for contributing. It looks like there are some tests failing - do you mind fixing those so that we can review this?

@finico
Copy link
Contributor Author

finico commented Jul 31, 2019

Hi @kaicataldo , thank you for quick response.
As I mentioned in the description, these tests were fixed in 416th pull request.
We can wait till it will be merged, then I will rebase my branch on upstream

lib/espree.js Outdated Show resolved Hide resolved
@platinumazure
Copy link
Member

Hi @finico, would you mind merging/rebasing upstream changes please? Thanks!

@finico
Copy link
Contributor Author

finico commented Aug 14, 2019

@platinumazure @kaicataldo done

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! I would like another team member to review before merging.

Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for contributing!

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks so much for contributing! I apologize for any confusion in our review comments as I think I had something slightly different in mind from @kaicataldo 😄

@platinumazure platinumazure merged commit 3f49224 into eslint:master Aug 15, 2019
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.

3 participants