Skip to content

Conversation

lumirlumir
Copy link
Member

@lumirlumir lumirlumir commented May 24, 2025

Prerequisites checklist

What is the purpose of this pull request?

Hello,

In this PR, I've completed implemention of getLocFromIndex() and getIndexFromLoc().

I based the implementation on the current JavaScript SourceCode class and incorporated the fast lookup algorithm discussed in eslint/eslint#19782.

The logic of getLocFromIndex() and getIndexFromLoc() is nearly identical to that in the JavaScript SourceCode class, with the key difference being that it accounts for the lineStart and columnStart values initialized in the constructor.


lineStart and columnStart

TextSourceCodeBase should accept lineStart and columnStart, as these can vary depending on the language, and both getLocFromIndex() and getIndexFromLoc() rely on them for accurate calculations. Here are some examples:

What changes did you make? (Give an overview)

I've completed implemention of getLocFromIndex() and getIndexFromLoc().

Related Issues

fixes: #166
refs: eslint/markdown#376, eslint/css#167, eslint/json#109, eslint/eslint#19782

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

Prerequisites

@nzakas nzakas moved this from Needs Triage to Implementing in Triage Jun 5, 2025
@lumirlumir lumirlumir requested a review from nzakas July 11, 2025 14:01
nzakas
nzakas previously approved these changes Jul 23, 2025
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. Would like @mdjermanovic to review before merging.

@nzakas nzakas moved this from Implementing to Second Review Needed in Triage Jul 23, 2025
@lumirlumir lumirlumir marked this pull request as draft July 30, 2025 10:18
@lumirlumir lumirlumir force-pushed the feat-add-support-for-getlocfromindex-and-getindexfromloc branch from 5bde96b to 92f1094 Compare August 2, 2025 09:45
@lumirlumir
Copy link
Member Author

lumirlumir commented Aug 2, 2025

@mdjermanovic

Thank you for your thorough review!

I've resolved all the comments and added the test cases you suggested in #212 (comment) and other comments.

The bug you pointed out in #212 (comment) was caused by the following code:

this.text.slice(this.#lineStartIndices.at(-1), index + 1)

Since I used the slice method with index + 1, it led to the error you mentioned.

I've now fixed this in 842dfb8, so the error no longer occurs.

The other private lazy methods already use the approach I implemented in 842dfb8, so the bug was only present in the #ensureLineStartIndicesFromIndex(index) method.

@lumirlumir lumirlumir marked this pull request as ready for review August 2, 2025 10:57
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! Leaving open for @nzakas to verify changes after his approval.

@lumirlumir
Copy link
Member Author

ping @nzakas

Copy link
Member

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

@nzakas nzakas merged commit a3588d8 into main Aug 20, 2025
19 checks passed
@nzakas nzakas deleted the feat-add-support-for-getlocfromindex-and-getindexfromloc branch August 20, 2025 14:48
@github-project-automation github-project-automation bot moved this from Second Review Needed to Complete in Triage Aug 20, 2025
@github-actions github-actions bot mentioned this pull request Aug 21, 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: Support getLocFromIndex() and getIndexFromLoc() methods for TextSourceCodeBase class
3 participants