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

Update: improve report location for no-space-in-parens #12364

Merged
merged 2 commits into from Oct 18, 2019

Conversation

@golopot
Copy link
Contributor

golopot commented Oct 3, 2019

What is the purpose of this pull request? (put an "X" next to item)

[x] Changes an existing rule

What changes did you make? (Give an overview)

Change the report location of no-space-in-parens. When option never, location is changed to that of the disallowed spaces. When option 'always', missing end location is added. This change fixes part of #12334, where missing end location causes VS code to render highlighting tildes in unexpected locations.

/* eslint no-space-in-parens: [2, 'never'] */
Math.random( foo)
//   ~~~~~~          vs code report location — before
//          ~        vs code report location — after
//         ~         report location — before (missing end location)
//          ~        report location — after

/* eslint no-space-in-parens: [2, 'always'] */
Math.random(foo )
//   ~~~~~~          vs code report location — before
//         ~         vs code report location — after
//         ~         report location — before (missing end location)
//         ~         report location — after

Is there anything you'd like reviewers to focus on?
No

@eslint eslint bot added the triage label Oct 3, 2019
Copy link
Member

kaicataldo left a comment

This mostly LGTM, thanks! Could we add some tests that have multiple spaces just so we're sure what the behavior should be then?

@golopot golopot requested a review from kaicataldo Oct 14, 2019
Copy link
Member

kaicataldo left a comment

LGTM, thanks!

Copy link
Member

mdjermanovic left a comment

LGTM, thanks! Tried in VS Code, highlighting looks good:

image

It might look a little strange that the locations in the messages are same for both left and right paren when there is nothing inside. Also, the message 'There should be no space before this paren' shows a location that is far from that paren. But I think it's okay.

@platinumazure platinumazure self-assigned this Oct 18, 2019
@platinumazure platinumazure added accepted and removed evaluating labels Oct 18, 2019
@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Oct 18, 2019

I'll champion. This is now accepted.

Copy link
Member

platinumazure left a comment

Looks good to me, thanks for contributing!

@platinumazure platinumazure merged commit 18a0b0e into eslint:master Oct 18, 2019
16 checks passed
16 checks passed
Verify Files
Details
Test (ubuntu-latest, 8.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 12.x)
Details
Test (windows-latest, 12.x)
Details
Test (macOS-latest, 12.x)
Details
Browser Test
Details
commit-message PR title follows commit message guidelines
Details
continuous-integration Build #20191003.3 succeeded
Details
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@golopot golopot deleted the golopot:location-space-in-parans branch Mar 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.