Add support for endLine and endColumn #709
Conversation
|
Rebased. |
Arcanemagus
force-pushed
the
eslint-full-range
branch
from
c374457 to
e8a5cb0
Compare
| @@ -61,3 +62,11 @@ function showError(givenMessage) { | |||
| dismissable: true | |||
| }); | |||
| } | |||
|
|
|||
| function validatePoint(textEditor, line, col) { | |||
There was a problem hiding this comment.
Should this be validateRange and do similar logic but just use TextBuffer::clipRange? It would save calling it twice...
Arcanemagus
force-pushed
the
eslint-full-range
branch
from
e8a5cb0 to
b616e09
Compare
|
Looks like |
Arcanemagus
force-pushed
the
eslint-full-range
branch
from
b616e09 to
8638185
Compare
| @@ -164,10 +166,16 @@ module.exports = { | |||
| } | |||
| } | |||
| let range | |||
| const msgLine = line - 1 | |||
| const msgCol = column ? column - 1 : column | |||
There was a problem hiding this comment.
This works fine, but it might be nice to be a little more explicit with something like:
const msgCol = column > 0 ? column - 1 : 0or even better (I think):
const msgCol = Math.max(0, column - 1)There was a problem hiding this comment.
Ooo, I like that idea.
There was a problem hiding this comment.
Actually, after looking at the code the reason I was passing it on directly if it was "falsey" was that rangeFromLineNumber will ignore it if it is undefined or null... meaning it will give us the whole line highlight that we want if there is only a line specified.
I'll make that clearer in the code though.
| if (endColumn && endLine) { | ||
| validatePoint(textEditor, msgLine, msgCol) | ||
| validatePoint(textEditor, endLine - 1, endColumn - 1) | ||
| range = [[msgLine, msgCol], [endLine - 1, endColumn - 1]] |
There was a problem hiding this comment.
Question: why are you subtracting 1 from the endColumn? I understand subtracting for endLine, since it is 1-based, but endColumn is 0-based.
There was a problem hiding this comment.
I assumed endColumn is 1-based like all the other points we get from ESLint... did they really make 1/4 of the points 0-based?!?
There was a problem hiding this comment.
column numbers are 0-based, line numbers are 1-based.
There was a problem hiding this comment.
Looks like rules have mixed 1-based and 0-based coords, but the results are all 1-based.
| range = Helpers.rangeFromLineNumber( | ||
| textEditor, line - 1, column ? column - 1 : column | ||
| ) | ||
| if (endColumn && endLine) { |
There was a problem hiding this comment.
Are we sure that a valid endColumn won't be 0? If we get an endColumn of zero, and an endLine of, say, 5, it seems like we should still use them. But I think this check will return false, right? Perhaps a safer thing to do is check typeof endColumn !== 'undefined'?
There was a problem hiding this comment.
That's what I get for falling in with the lazy crowd that abuses that hidden check, thanks!
| const buffer = textEditor.getBuffer() | ||
| // Clip the given point to a valid one, and check if it equals the original | ||
| if (!buffer.clipPosition([line, col]).isEqual([line, col])) { | ||
| throw new Error(`${line}:${col} isn't a valid point!`) |
There was a problem hiding this comment.
I'm not convinced that throwing an error is the right thing to do here. We could be more graceful and just use the clipped position. I suppose the positive here is that we might be able to catch ESLint errors, whereas without throwing an error people will just live with the possibly slightly-inaccurate location marking. Pros and cons either way, but personally I think I lean towards not throwing an error. Curious to hear your thoughts.
There was a problem hiding this comment.
This should be validating the point in a similar manner to how rangeFromLineNumber() does. Where this is being called over here is wrapped in the same try/catch that the call to rangFromLineNumber() is, so this throw will be caught and handled in the exact same manner as any other invalid point we run into.
Arcanemagus
force-pushed
the
eslint-full-range
branch
from
8638185 to
c3f605b
Compare
Add support for creating a full range from an ESLint message if it specifies an `endLine` and `endColumn`. Also adds a `validatePoint` helper function to check that these coordinates given from ESLint are valid since we no longer have the checking built into `helpers.rangeFromLineNumber`.
Properly check whether the positions are undefined, instead of just "falsey".
Also updates compiled code, since `babel` changed it's mind again.
Arcanemagus
force-pushed
the
eslint-full-range
branch
from
c3f605b to
192925d
Compare
In eslint/eslint#6640, released in
eslint@3.1.0, ESLint added the ability for a rule to return a full range instead of just a single point. This PR adds support for that.Merging of this is currently blocked however as no core rule currently actually returns a range (eslint/eslint#3307), so we have nothing to validate against.