-
-
Notifications
You must be signed in to change notification settings - Fork 11
feat: Add token methods to JSONSourceCode #112
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
Conversation
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.
Could you take a look at the CI failure? Bun CI is currently throwing an error.
Also, I’ve left comments on a few parts!
Bun test looks like it was a temporary issue. |
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.
Overall looks good to me! I've left a few comments on some missing parts.
https://eslint.org/docs/latest/extend/custom-rules#deprecated-sourcecode-methods So I think we shouldn't have these methods here, but an option to include comments in |
Co-authored-by: 루밀LuMir <rpfos@naver.com>
Co-authored-by: 루밀LuMir <rpfos@naver.com>
Co-authored-by: 루밀LuMir <rpfos@naver.com>
Co-authored-by: 루밀LuMir <rpfos@naver.com>
@mdjermanovic good call. Don't know how I missed that. 👍 |
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.
LGTM, Thanks!
I’ve left a minor typo fix in the comments. Would like @mdjermanovic to verify the changes before merging.
Co-authored-by: 루밀LuMir <rpfos@naver.com>
Currently, the CI fails for the same reason that happened in eslint/css#182. I've looked into the Prettier repository and found that there is a fix ongoing for this:
So, if this new feature addition is not in a hurry, I think we can wait until the Prettier bug fix is released. |
I've fixed the Prettier error so this should be ready for review. |
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.
Re-approving. Still pending review from @mdjermanovic before merging.
ping @mdjermanovic |
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.
LGTM, thanks! Leaving open for @lumirlumir to re-review.
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 think we can merge this because the CI failures are known to not be related. |
Prerequisites checklist
What is the purpose of this pull request?
Adds token methods to
JSONSourceCode
.What changes did you make? (Give an overview)
Added the following methods to
JSONSourceCode
:getTokenBefore()
getTokenAfter()
getTokenOrCommentBefore()
getTokenOrCommentAfter()
Also added tests for these methods.
Additionally, I changed
comments
to a getter so it can be lazily initialized.(The tokens array from Momoa contains both tokens and comments, so it makes sense to lazily initialize it all together.)
Related Issues
fixes #83
Is there anything you'd like reviewers to focus on?
I wasn't sure if we needed
getTokenOrCommentBefore()
orgetTokenOrCommentAfter()
. These appear as convenience methods onSourceCode
in theeslint
package, butgetTokenBefore()
andgetTokenAfter()
also have a second argument that lets you specify whether or not to include comments. We could go that way here and eliminate two methods, or we could keep these as-is. I'm not sure what the better path is as we'd likely want to encourage other languages to match whatever we decide on here.