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

Stop hiding the column number of ESLint errors #6980

Merged
merged 2 commits into from Oct 1, 2019
Merged

Stop hiding the column number of ESLint errors #6980

merged 2 commits into from Oct 1, 2019

Conversation

justingrant
Copy link
Contributor

Currently, CRA's npm start output only reports the line number of ESLint errors but hides the column numbers. This makes it harder for IDE users to leverage the time-saving features provided by IDEs like VSCode to deal with build errors:

  • devs can't one-click to navigate from the build error message to the exact spot in the code where the problem is.
  • the code editor won't show the little red sqiggly line under the exact spot in the code where the problem is.

This PR removes one line of code (originally introduced in #5174 last year) that removes column numbers from ESLint errors. Here's the current code. As you can see, it has no side effects-- it removes column numbers but doesn't do any other changes to the string.

// Remove columns from ESLint formatter output (we added these for more
// accurate syntax errors)
message = message.replace(/Line (\d+):\d+:/g, 'Line $1:');

Here's what existing errors look like:

./src/Calendar.tsx
  Line 19:  Unnecessary escape character: \.  no-useless-escape

Here's what errors will look like after this PR:

./src/Calendar.tsx
  Line 19:21:  Unnecessary escape character: \.  no-useless-escape

@justingrant
Copy link
Contributor Author

BTW, I couldn't get the tests running on my Mac, so if this change breaks any tests then I'll fix 'em once Travis tells me where the breaks are. ;-)

@justingrant
Copy link
Contributor Author

Thanks @ianschmitz. Sorry for all the Travis build spam. Last night Travis builds were all failing with network errors, e.g.

Command failed: yarnpkg install --mutex network
    error An unexpected error occurred: "http://localhost:4873/@types%2freact: no such package available".

Looks like Travis's problem was fixed by the time I tried to force-push the same commit again this morning. Thanks again!

@stale
Copy link

stale bot commented Jun 3, 2019

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Jun 3, 2019
@justingrant
Copy link
Contributor Author

Not stale. Hi @ianschmitz - You approved this PR a month ago, but it's still not merged. Is there anything I can do to help get it merged? And while we're talking stale PRs, #7022 to fix VSCode debugging is still waiting for a review. Is there anything I can do to help move it along?

@stale stale bot removed the stale label Jun 3, 2019
@bugzpodder bugzpodder added this to the 3.0.2 milestone Jun 19, 2019
@mrmckeb mrmckeb modified the milestones: 3.1, 3.2 Aug 9, 2019
@mrmckeb mrmckeb added this to In progress in v3.3 via automation Aug 9, 2019
@mrmckeb
Copy link
Contributor

mrmckeb commented Aug 9, 2019

Hi @justingrant, this would be great for the next release. Can you rebase this and push again, so it runs through the updated CI - we can then get it merged.

@mrmckeb mrmckeb self-assigned this Aug 9, 2019
@bugzpodder bugzpodder closed this Aug 9, 2019
v3.3 automation moved this from In progress to Done Aug 9, 2019
@bugzpodder bugzpodder reopened this Aug 9, 2019
v3.3 automation moved this from Done to In progress Aug 9, 2019
@justingrant
Copy link
Contributor Author

@mrmckeb - looks like @bugzpodder triggered a CI run (which passed except for a Windows 10 installation issue which I assume is unrelated to my PR). Do you still need me to rebase and push again?

@mrmckeb
Copy link
Contributor

mrmckeb commented Aug 11, 2019

Yes please @justingrant, the PR is quite far out of date (which is our fault, sorry).

@justingrant
Copy link
Contributor Author

Hi @mrmckeb - OK rebase is done. While we're talking about old PRs, what's the latest status on #7022? Anything I need to do to get that PR reviewed? Should I rebase that PR too?

@ianschmitz ianschmitz removed this from In progress in v3.3 Oct 1, 2019
@ianschmitz ianschmitz modified the milestones: 3.2, 3.1.3 Oct 1, 2019
@ianschmitz ianschmitz merged commit 85aac9b into facebook:master Oct 1, 2019
@lock lock bot locked and limited conversation to collaborators Oct 6, 2019
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.

None yet

5 participants