Skip to content

feat: Add token methods to JSONSourceCode#112

Merged
nzakas merged 12 commits into
mainfrom
issue83
Jul 7, 2025
Merged

feat: Add token methods to JSONSourceCode#112
nzakas merged 12 commits into
mainfrom
issue83

Conversation

@nzakas

@nzakas nzakas commented Jun 18, 2025

Copy link
Copy Markdown
Member

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() or getTokenOrCommentAfter(). These appear as convenience methods on SourceCode in the eslint package, but getTokenBefore() and getTokenAfter() 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.

@lumirlumir lumirlumir left a comment

Copy link
Copy Markdown
Member

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!

Comment thread package-lock.json Outdated
Comment thread tests/languages/json-source-code.test.js Outdated
@github-project-automation github-project-automation Bot moved this to Needs Triage in Triage Jun 19, 2025
@lumirlumir lumirlumir moved this from Needs Triage to Implementing in Triage Jun 19, 2025
@lumirlumir
lumirlumir requested a review from a team June 19, 2025 09:16
@nzakas

nzakas commented Jun 19, 2025

Copy link
Copy Markdown
Member Author

Bun test looks like it was a temporary issue.

@lumirlumir lumirlumir left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/languages/json-source-code.js
Comment thread src/languages/json-source-code.js Outdated
Comment thread src/languages/json-source-code.js Outdated
Comment thread tests/languages/json-source-code.test.js
Comment thread tests/languages/json-source-code.test.js Outdated
Comment thread tests/languages/json-source-code.test.js Outdated
Comment thread tests/languages/json-source-code.test.js
Comment thread tests/languages/json-source-code.test.js
Comment thread tests/languages/json-source-code.test.js Outdated
Comment thread tests/languages/json-source-code.test.js Outdated
@mdjermanovic

mdjermanovic commented Jun 20, 2025

Copy link
Copy Markdown
Member

I wasn't sure if we needed getTokenOrCommentBefore() or getTokenOrCommentAfter(). These appear as convenience methods on SourceCode in the eslint package, but getTokenBefore() and getTokenAfter() 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.

getTokenOrCommentBefore() and getTokenOrCommentAfter() are deprecated in the eslint package:

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 getTokenBefore() and getTokenAfter().

nzakas and others added 4 commits June 20, 2025 15:11
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>
@nzakas

nzakas commented Jun 20, 2025

Copy link
Copy Markdown
Member Author

@mdjermanovic good call. Don't know how I missed that. 👍

lumirlumir
lumirlumir previously approved these changes Jun 25, 2025

@lumirlumir lumirlumir left a comment

Copy link
Copy Markdown
Member

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.

Comment thread tests/languages/json-source-code.test.js Outdated
@lumirlumir lumirlumir moved this from Implementing to Second Review Needed in Triage Jun 25, 2025
Co-authored-by: 루밀LuMir <rpfos@naver.com>
@lumirlumir

Copy link
Copy Markdown
Member

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.
If not, we can fix the CI like we did in eslint/css#182.

@nzakas

nzakas commented Jun 27, 2025

Copy link
Copy Markdown
Member Author

I've fixed the Prettier error so this should be ready for review.

lumirlumir
lumirlumir previously approved these changes Jun 28, 2025

@lumirlumir lumirlumir left a comment

Copy link
Copy Markdown
Member

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.

@nzakas

nzakas commented Jul 1, 2025

Copy link
Copy Markdown
Member Author

ping @mdjermanovic

Comment thread src/languages/json-source-code.js Outdated
Comment thread src/languages/json-source-code.js

@mdjermanovic mdjermanovic left a comment

Copy link
Copy Markdown
Member

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.

@lumirlumir lumirlumir left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, Thanks!

Pending until the CI error is resolved.

(Maybe #117 could be a workaround.)

@lumirlumir lumirlumir moved this from Second Review Needed to Merge Candidates in Triage Jul 7, 2025
@nzakas

nzakas commented Jul 7, 2025

Copy link
Copy Markdown
Member Author

I think we can merge this because the CI failures are known to not be related.

@nzakas
nzakas merged commit de3e150 into main Jul 7, 2025
17 of 32 checks passed
@nzakas
nzakas deleted the issue83 branch July 7, 2025 14:03
@github-project-automation github-project-automation Bot moved this from Merge Candidates to Complete in Triage Jul 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Complete

Development

Successfully merging this pull request may close these issues.

Change Request: Missing TokenStore methods in JSONSourceCode breaks compatibility with ESLint utilities

3 participants