Skip to content

Conversation

lumirlumir
Copy link
Member

@lumirlumir lumirlumir commented May 14, 2025

Prerequisites checklist

What is the purpose of this pull request?

In this PR, I've updated the @eslint/plugin-kit and @eslint/core dependencies to add support for the getLocFromIndex and getIndexFromLoc methods.

Since these methods were already well tested in the rewrite repository, I've only added a few simple tests to verify their behavior.

What changes did you make? (Give an overview)

In this PR, I've updated the @eslint/plugin-kit and @eslint/core dependencies to add support for the getLocFromIndex and getIndexFromLoc methods.

Related Issues

Ref: eslint/rewrite#212

Is there anything you'd like reviewers to focus on?

N/A

Prerequisite

@nzakas
Copy link
Member

nzakas commented May 14, 2025

Before you get too far on this, is this function currently needed? As discussed in eslint/rewrite#166, this is an expensive operation and I don't think we want to incorporate it unless we absolutely need it.

@lumirlumir
Copy link
Member Author

lumirlumir commented May 14, 2025

@nzakas

I thought this method could replace the findOffsets method used throughout the rules in @eslint/markdown, so I saw it as both a refactor and a new feature.

image

It’s currently used in two rules, and I use this kind of logic in my own rules as well (for context, please see https://github.com/lumirlumir/npm-eslint-plugin-mark/blob/main/packages/eslint-plugin-mark/src/core/ast/text-handler/text-handler.js).

If this feature isn’t necessary for now, I’ll go ahead and close it.


(If it is fine, I would like to keep exploring and look for a more performant solution.)

@nzakas
Copy link
Member

nzakas commented May 14, 2025

@lumirlumir ah, I forgot about that. Gotcha. 👍 If you're going to go through the trouble of investigating, then I'd suggest doing it on TextSourceCodeBase so every project can benefit.

@lumirlumir
Copy link
Member Author

lumirlumir commented May 14, 2025

@nzakas Thanks for understanding!

I’ll keep this PR open for now, move on to implementing TextSourceCodeBase, and then come back to this one afterward.

Can I assign myself for the issue I've created? eslint/rewrite#166

@nzakas
Copy link
Member

nzakas commented May 14, 2025

Go ahead.

@nzakas
Copy link
Member

nzakas commented May 14, 2025

The other thing to keep in mind is that findOffsets() is not always used from the beginning of the document. Like in this case:

const { lineOffset, columnOffset } = findOffsets(

It's finding the offset inside a given node's text.

@lumirlumir lumirlumir changed the title feat: add support for getLocFromIndex feat: add support for getLocFromIndex and getIndexFromLoc Jun 6, 2025
Copy link
Contributor

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

"dependencies": {
"@eslint/core": "^0.14.0",
"@eslint/plugin-kit": "^0.3.1",
"@eslint/plugin-kit": "^0.4.0",
Copy link
Member

Choose a reason for hiding this comment

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

As @eslint/plugin-kit v0.4.0 has been released, we could now continue work here?

Copy link
Member Author

Choose a reason for hiding this comment

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

There was some delay on my end with the markdown plugin. I'll work on it soon! 👍

describe("getLocFromIndex()", () => {
it("should convert index to location correctly", () => {
const text = "foo\nbar\r\nbaz";
const markdownSourceCode = new MarkdownSourceCode({
Copy link
Member Author

Choose a reason for hiding this comment

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

I've used the markdownSourceCode name here because the following errors occur:

110:10  error  'sourceCode' is already declared in the upper scope on line 63 column 6  no-shadow
173:10  error  'sourceCode' is already declared in the upper scope on line 63 column 6  no-shadow

@lumirlumir lumirlumir marked this pull request as ready for review September 24, 2025 09:09
@github-project-automation github-project-automation bot moved this to Needs Triage in Triage Sep 24, 2025
@lumirlumir lumirlumir moved this from Needs Triage to Implementing in Triage Sep 24, 2025
Copy link
Member

@mdjermanovic mdjermanovic 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!

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.

3 participants