feat: add support for getLocFromIndex and getIndexFromLoc
#212
Conversation
…c' of https://github.com/eslint/rewrite into feat-add-support-for-getlocfromindex-and-getindexfromloc
There was a problem hiding this comment.
It took a while, but I’ve finally finished implementing these two methods 😄
I’ve carefully reviewed the lazy calculation and addressed all the comments.
There might still be room for improvement—if so, I’d be happy to take a look.
(I didn’t go with the caching approach, as it turned out to be slightly slower in eslint/eslint#19782)
| ); | ||
| } | ||
|
|
||
| const rootNodeLoc = this.getLoc(this.ast); |
There was a problem hiding this comment.
Now, we retrieve lineStart and columnStart information from the AST.
|
I've addressed all the comments and done a bit of refactoring to make the code clearer. 😄 |
| */ | ||
| #parseText(text) { | ||
| // Create a new RegExp instance to avoid lastIndex issues. | ||
| const match = structuredClone(this.#lineEndingPattern).exec(text); |
There was a problem hiding this comment.
It doesn't seem like this is necessary, or at least, we should be able to reuse just one copy of the regex that is passed into the constructor. I don't think we need a new copy every time this function is called.
Also, the best way to duplicate a regex would be RegExp(pattern). structuredClone is really intended for cloning data structures not class instances.
| * @param {string} text The source text to parse into lines. | ||
| * @returns {boolean} `true` if the text was successfully parsed into lines, `false` otherwise. | ||
| */ | ||
| #parseText(text) { |
There was a problem hiding this comment.
This isn't actually parsing text, so the name is misleading.
Maybe findNextLine is more accurate?
(Also, the JSDoc needs to be updated to reflect this.)
| Boolean(additionalLinesNeeded--) && | ||
| (match = lineEndingPattern.exec(text)) | ||
| this.#parseText(this.text.slice(this.#lineStartIndices.at(-1))) && | ||
| Boolean(additionalLinesNeeded--) |
There was a problem hiding this comment.
The intent here isn't very clear. Can we do this something like this?
| Boolean(additionalLinesNeeded--) | |
| additionalLinesNeeded-- > 0 |
Although, I think it would be even clearer if we could decrement inside the start of the loop instead of in the condition.
Prerequisites checklist
What is the purpose of this pull request?
Hello,
In this PR, I've completed implemention of
getLocFromIndex()andgetIndexFromLoc().I based the implementation on the current JavaScript
SourceCodeclass and incorporated the fast lookup algorithm discussed in eslint/eslint#19782.The logic of
getLocFromIndex()andgetIndexFromLoc()is nearly identical to that in the JavaScriptSourceCodeclass, with the key difference being that it accounts for thelineStartandcolumnStartvalues initialized in the constructor.lineStartandcolumnStartTextSourceCodeBaseshould acceptlineStartandcolumnStart, as these can vary depending on the language, and bothgetLocFromIndex()andgetIndexFromLoc()rely on them for accurate calculations. Here are some examples:What changes did you make? (Give an overview)
I've completed implemention of
getLocFromIndex()andgetIndexFromLoc().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
getLocFromIndexeslint#19782