-
-
Notifications
You must be signed in to change notification settings - Fork 75
Breaking: Allow comment scanner to rescan tokens (fixes #216) #219
Conversation
LGTM |
@JamesHenry Can you give this a review when you get a chance, no rush. Maybe there is a better way to find a container token or we could use the typescript format scanner? |
68fcf34
to
416b85e
Compare
LGTM |
416b85e
to
55e15cd
Compare
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs major rebase (or perhaps a fresh branch would be easier?)
55e15cd
to
f1649b0
Compare
LGTM |
LGTM |
@JamesHenry its rebased now. I but the comment code in a separate file |
Does the following parse correctly:
|
I added a test case for it. This is the result https://github.com/eslint/typescript-eslint-parser/blob/a5e2293d5d44c43b26b18e93eac27013ae0fe766/tests/fixtures/comments/line-comment-with-block-syntax.result.js I changed the logic so line comments only remove //. Block comments only remove /* and */ |
lib/convert-comments.js
Outdated
@@ -0,0 +1,147 @@ | |||
/** | |||
* @fileoverview Convert comment using TypeScript token scanner | |||
* @author Nicholas C. Zakas |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically I wrote all the comment code 😄
lib/ast-converter.js
Outdated
@@ -69,8 +70,8 @@ module.exports = (ast, extra) => { | |||
/** | |||
* Add the comment nodes to the AST (that were parsed separately in parser.js) | |||
*/ | |||
if (extra.comment || extra.attachComment) { | |||
estree.comments = extra.comments || []; | |||
if (extra.comment) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this was redundant until now, but this is breaking change - prettier currently uses the attachComment
version and will need to be updated.
I think we should still make the change, but we will need to update it to Breaking
This looks great @soda0289, thanks for all digging around |
Template strings and regex patterns need to be rescaned to not scan tokens as comments. Fix how comment tokens are removed.
a5e2293
to
2044025
Compare
LGTM |
@JamesHenry Thanks for the review. Made the recommended changes and fixed a comment in parser.js. |
Template strings and regex patterns need to be rescaned to not scan tokens as comments. I aslo re-factored the comment code and removed the attachComment option since it is no longer needed.
The code pattern was taken from here:
https://github.com/Microsoft/TypeScript/blob/2b10611fbfb63c4fe24d865dc275fec016af003e/src/services/formatting/formattingScanner.ts
I'm not sure if there is a better way to find the container token node. We could use a list of scanned tokens then search through it for a possible container but I think we need to know the node and not the token to find a container. The method I use currently walks the ast searching for a container node.