-
Notifications
You must be signed in to change notification settings - Fork 73
feat: add support for getLocFromIndex
and getIndexFromLoc
#376
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
base: main
Are you sure you want to change the base?
Conversation
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. |
I thought this method could replace the 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.) |
@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 |
@nzakas Thanks for understanding! I’ll keep this PR open for now, move on to implementing Can I assign myself for the issue I've created? eslint/rewrite#166 |
Go ahead. |
The other thing to keep in mind is that Line 71 in cb5a956
It's finding the offset inside a given node's text. |
getLocFromIndex
getLocFromIndex
and getIndexFromLoc
Prerequisites checklist
What is the purpose of this pull request?
Working in progress.
What changes did you make? (Give an overview)
Related Issues
Is there anything you'd like reviewers to focus on?
Prerequisite
getLocFromIndex
andgetIndexFromLoc
rewrite#212getLocFromIndex
eslint#19782