Handle errors as a lint message #1015
Conversation
Since both function calls are using the same buffer, just change the function to accept a TextBuffer and grab it before calling the function.
That function has 10 parameters, change it to an object so accidentally re-ordering them won't cause major issues.
The only other code that could throw in that block is the `validateRange` function, which would be the same issue (invalid point), so it should be handled in the same manner.
This function isn't actually used any more and should have been deleted back in #889.
Translate all errors coming from ESLint into a lint message instead of throwing them out to be caught by Linter's generic error catching mechanism. This allows us to present them in a much cleaner manner and gives the user immediate feedback on what is going wrong. Fixes #387.
|
Love the idea. Not a big fan of the full page stack trace. That looks even more invasive than throwing an Atom error. Is there a plan to be able to suppress the stack until user explicitly requests it? Edit: I think I misread. I guess that's exactly what you're doing already. Sweet! 👍 👍 👍 |
|
Updated the description to make it a bit clearer that the stack trace is only shown in the The generally seen message ( |
src/helpers.js
Outdated
| @@ -150,10 +133,25 @@ export async function generateDebugString(worker) { | |||
| return details.join('\n') | |||
| } | |||
|
|
|||
| const generateInvalidTrace = async ( | |||
| export async function handleError(textEditor, error) { | |||
There was a problem hiding this comment.
Whoops, wrote this as async initially, but it doesn't actually need to be.
This function isn't async, don't mark it as such.
There was a problem hiding this comment.
This is a nice user-experience improvement for sure. I don't think it should keep us from proceeding, but I do notice that the formatting isn't great in Nuclide (html tags are shown):
Other than that small concern and one minor question, I think this looks great and is much nicer than getting the error pop-ups.
| msgLine, msgCol, msgEndLine, msgEndCol, | ||
| eslintFullRange, filePath, textEditor, ruleId, message, worker | ||
| ) => { | ||
| }) => { |
There was a problem hiding this comment.
😻
10 arguments is verging on ridiculous, good call making it an object.
| ) { | ||
| // This isn't an invalid point error from `generateRange`, re-throw it | ||
| throw err | ||
| } |
There was a problem hiding this comment.
Question
We are guaranteed that ruleId will be a string or undefined, right? I only ask, because if it's truthy but something other than a string, ruleURI() will throw a TypeError.
There was a problem hiding this comment.
It comes straight from ESLint itself, so I'm not sure. I'll move that out of the try though just in case.
|
The formatting issue would be a bug with Nuclide, it's not our fault they can't properly render messages 😛. ( |
|
Looks like they made the same mistake in |
On the off chance that ESLint feeds us something that is truthy that isn't a rule it could throw a `TypeError`, so move this out of the `try` block for invalid points.
Arcanemagus
force-pushed
the
arcanemagus/better-error-handling
branch
from
610b83c
to
a93881a
Compare
|
I feel like Thrown Errors should be distinguished in some way from Linter Errors (even if message details not expanded). Perhaps just prepend "Atom" or "Thrown" to the Severity? So you would see "Atom Error" for thrown errors and just "Error" for linting errors. |
|
The errors aren't from Atom though? As stated in the message, the error is coming from ESLint. Errors that are thrown outside of the handled sections are still bubbled up to Linter/Atom. |
|
Okay I understand. So it's an ESlint Error, but that name seems pretty meaningless since the linter errors are also from ESLint. "Thrown Error" or similar still makes to me here. I guess it's not hugely important. But if running in CLI, your output would be very very different from a thrown error vs a lint error. I feel like at least some little thing to clearly acknowledge this difference would be helpful. |
|
Do you have a better proposed text than the current |
|
Hmm. I typed a few ideas out, but none of them seem any better except this: Stars are a text version of highlighting. We're just trying to point out that "Hey, this is a special error." It's typically fatal and will prevent any further linting, so it should be easy to tell instantly that it's not a typo in this file file that's causing it. Stars in the Description text seems sufficient to me as visually indicating the extra severity, actually better than my initial proposal. |
Handle errors as a lint message
Translate all errors coming from ESLint into a lint message instead of throwing them out to be caught by Linter's generic error catching mechanism. This allows us to present them in a much cleaner manner and
gives the user immediate feedback on what is going wrong.
Also includes several cleanups related to error handling to get a more unified experience with errors.
Before
Errors were only shown as an Atom error notification from Linter's generic catch-all:
Repeated lint requests would spam the message on the user's screen, providing a rather poor experience.
After
Errors coming from ESLint will show as a message in Linter, with the expandable message details showing the full stack trace:
Linter message:
Full details:
Fixes #387.