-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
Open editor to exact column from build error overlay #3465
Conversation
Not opposed to it. Main reason was to keep the message looking focused. If you can find a way to make it look nice (and not distracting) then ok. |
@@ -256,6 +264,10 @@ function launchEditor(fileName, lineNumber) { | |||
return; | |||
} | |||
|
|||
// colNumber is optional, but should be a number | |||
// default is 1 | |||
colNumber = parseInt(colNumber, 10) || 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it not a number by this point? Can we assume it's always either missing or a number?
I would feel better if we forced line and column to be valid numbers in this function and threw if they're not. And validate/parse arguments in the middleware instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the middleware, we directly pass line and column from the query string. So they are strings. But I agree with you. Let's validate/parse in the middleware. We can expect positive integers(number might be a decimal or negative integer) for this function and throw an error otherwise. Or do we need that? since this function is only used internally. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed with 2e346d4
Will something like follows work? Line 16: Col:8 'App' is not defined react/jsx-no-undef Alternatively, we can keep column number hidden in the message and only use it to open editor at the right position. |
Can you show screenshot of how it looks with columns? |
36fa960
to
2e346d4
Compare
* Open editor to exact column from build error overlay * Update launch editor validations
Fixes #3358
Only support babel syntax errors with Atom, Sublime, Notepad++, Emacs, and VSCode. Welcome support for other editors.
@gaearon Any particular reason to ignore the column number from eslint errors? If not we can change the message format and support eslint errors as well.