Smarter ranges #43
Smarter ranges #43
Conversation
| // We want msgCol to remain undefined if it was initially so | ||
| // `rangeFromLineNumber` will give us a range over the entire line | ||
| const msgCol = typeof x.column === 'undefined' ? x.column : x.column - 1; | ||
| range = rangeFromLineNumber(textEditor, msgLine, msgCol); |
There was a problem hiding this comment.
Note that if an invalid point is provided this will throw an error, you may want to catch and handle this somehow.
ESLint has had a variety of issues with rules in the past, many fixed due to this catching the bugs and users reporting them 😛.
There was a problem hiding this comment.
I may be wrong, but I thought Atom has a general try/catch for each package that will result in an error notification and the ability for the user to submit the error to the package. If this is incorrect, please let me know and I will add some sugar over this; otherwise, I think loudly failing on range errors is better than silently failing as linter-estlint does. As you mentioned, those bugs should be brought to someones attention, after all.
There was a problem hiding this comment.
linter-eslint catches the error and makes it more presentable (showing rule name, originally requested position, and message), Atom will simply show what rangeFromLineNumber throws, which is admittedly somewhat cryptic to users:
Column start (x) greater than line length (y) for line z
As a note, see here for my current plans on making this less of a pain for users, I just haven't had time to implement it yet.
There was a problem hiding this comment.
Oh, gotcha. Yeah, i'll throw in some formatting, can't hurt.
| range = rangeFromLineNumber(textEditor, msgLine, msgCol); | ||
| } | ||
| } catch (err) { | ||
| throw new Error(`Cannot mark location in editor for (${x.ruleId}) - (${x.message}) at line (${x.line}) column (${x.column})`); |
There was a problem hiding this comment.
This is not very actionable. It would be helpful if it would point out that it's an issue with one of the rules and what the users should do (report it to the specific rule).
There was a problem hiding this comment.
This would only be throw if there is a problem here, not with a rule. If that function is called, we have determined eslint did not provide endColumn / endLine and we are assuming a range so we can have some type of underline support.
Edit: Also, since this isn't immediately obvious, I just grabbed that error message from linter-eslint - I can make whatever modification you want in order to get this merged, though.
There was a problem hiding this comment.
@jonathandelgado What @sindresorhus means is that rangeFromLineNumber throws when it is fed an invalid point, the only way it should be throwing is if an ESLint rule has a bug in it. A great example of this is how no-multiple-empty-lines was broken and returning the wrong point between versions v1.8.0 and v3.4.0. It was fixed in v3.5.0 (due to getting reported in linter-eslint).
The problem with a broken rule like that throwing is that it happens on every edit for on the fly linters, hence why we plan to make it less intrusive. I like your idea of pointing users to where to report it as well @sindresorhus!
There was a problem hiding this comment.
Oh, gotcha, sorry for the misunderstanding. Any suggestions on that implementation? Not sure how to redirect Atom errors to anything other than the package that threw it, in respect to the automatic reporting feature.
There was a problem hiding this comment.
I would imagine something along the lines of "This is likely an issue with ESLint, please file an issue there", but @sindresorhus would be the one to answer what he wants here.
You've already "redirected" the error by catching it, you just need to change what happens with it from there. My plan is to add a fake "message" to linter with the relevant text.
| // taken from linter-eslint | ||
| let range; | ||
| const msgLine = x.line - 1; | ||
| try { |
There was a problem hiding this comment.
The try/catch should be around the rangeFromLineNumber call. No point in try/catching more than necessary.
|
@jonathandelgado Mind updating to what @Arcanemagus suggested in #43 (comment) ? |
|
@sindresorhus Sorry for the delay, just one of those weeks. Does this look good to you? |
|
Thank you :) |
Grabbed the following features from
linter-eslint, here. Prior to this PR, this plugin did not have linter underline support; adding the ability to assume ranges when eslint doesn't provide a range fixes that. Additionally, if there is anendColumnandendLinepresent, it will use that instead of assuming.endColumnandendLineatom-linterfor range assumptionResolves #28