-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix: use correct positions for ranges #1672
Conversation
Can you explain “There was no way to force PostCSS to use ranges”? For instance, by showing the error when developer need to use |
Sure thing; for example: result.warn('message', { node, index, endIndex })
result.warn('message', { node, word }) would use ranges. But if you wanted to infer a range for the whole node without providing an index, you wouldn't be able to: result.warn('message', { node }) would return a single position and result.warn('message', { node, inferRange: true }) |
What downsides do you see in enabling I like the idea of changing |
I was thinking of the case: result.warn(message, { node, index }) With current behaviour this would warn for a specific index. But if the range behaviour was default, it would warn for the range from the index to the end of the node. I wasn't sure if this would be desired in some instances. |
Yeap, in the case of Can we remove |
I think that's a good idea. How should we handle the case when the user wants to have a range and is only providing a start index since they want the range to extend to the end of the node? |
Do we generate
Hm. Can you show an example of such warning? |
Yes, it currently does so. I also updated the word warning test in the original PR: b1583d9#diff-f4ad3511d3af2ff0b7ae47b50c500dd3609bc42a6b74ed01ff3b3befec547a21R72-R78
For example, if we wanted to generate a warning for: a {
color: red ;
/* ^^^^^^^ */
} or: a { /* starting at brace */
color: red;
} /* ending at end */ If cases like these can be handled without needing an extra check, maybe I just don't know about it? |
One thing I was just thinking about: the end position for a warning is exclusive, as per the LSP specification. So if a warning is emitted for a single position, shouldn't it also get an end line and end column position? i.e. if the warning is for only the single character at line 1 col 2 the end position would be line 1 col 3. |
What do you think about explicit API like?
Yeap, I like the idea. More consistency. |
I really like this! For consistency, maybe we could also accept the start as a position? Otherwise, it might be strange only to accept an index for start but a position or index for end.
I'll make the appropriate change shortly. |
Sure! |
Here's the API with the latest commit: result.warn(message, { index }) // -> one character warning at index
result.warn(message, { endIndex }) // -> warning starts at node start, ends at endIndex
result.warn(message, { index, endIndex }) // -> warning starts at index, ends at endIndex
result.warn(message, { start }) // -> warning starts at start, ends at node end
result.warn(message, { end }) // -> warning starts at node start, ends at end
result.warn(message, { start, end }) // -> warning starts at start, ends at end
result.warn(message, { word }) // -> warning starts at word location, ends at word index + length Using the node's end is a little different, however, since result.warn(message, { index, end: decl.source.end }) you'd use result.warn(message, { index, end: { line: decl.source.end.line, column: decl.source.end.column + 1 } }) The alternative was to do this automatically in result.warn(message, { start: decl.positionBy({ index: 0 }) }) |
Sorry, can I ask you to rebase branch and update tests to |
Great. I am planing to release this release today or tomorrow. Do you need some time to test it or do we ready to go? |
It seems to be working well enough in stylelint/stylelint#5725! |
While testing range support using Stylelint, I noticed a couple of problems:
endIndex
orword
weren't providedThis patch fixes the first by using the node's end position as the default for end. The second problem is addressed by adding a new option,
inferRange
, which forces PostCSS to infer ranges when a word orendIndex
aren't provided. I thought to do it this way so that existing code elsewhere won't get ranges if it only provides a node unless it opts into it since I'm not sure if ranges would always be desired. However, if you disagree, I could be convinced otherwise to make ranges default regardless.