Skip to content
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

Merged
merged 2 commits into from
Jan 9, 2018

Conversation

tharakawj
Copy link
Contributor

@tharakawj tharakawj commented Nov 19, 2017

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.

@gaearon
Copy link
Contributor

gaearon commented Nov 19, 2017

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;
Copy link
Contributor

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.

Copy link
Contributor Author

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. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with 2e346d4

@tharakawj
Copy link
Contributor Author

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.

@gaearon
Copy link
Contributor

gaearon commented Nov 26, 2017

Can you show screenshot of how it looks with columns?

@tharakawj
Copy link
Contributor Author

Actually, I meant something like this.
screen shot 2017-11-27 at 1 39 17 am

Or we can bold the text to make it clear.
screen shot 2017-11-27 at 1 38 44 am

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants