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 location for semi and comma-dangle #12380

Merged
merged 4 commits into from Nov 18, 2019
Merged

Update: improve location for semi and comma-dangle #12380

merged 4 commits into from Nov 18, 2019

Conversation

golopot
Copy link
Contributor

@golopot golopot commented Oct 5, 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 for semi and comma-dangle. Fixes part of #12334 .

/* eslint semi: [2, 'always'] */
const a = Math.random
//             ~~~~~~    vscode highlight ⁠— before
//                   ~   vscode highlight ⁠— after

/* eslint comma-dangle: [2, 'always-multiline'] */
[
  Math.random
]
//     ~~~~~~    vscode highlight ⁠— before
//           ~   vscode highlight ⁠— after

Is there anything you'd like reviewers to focus on?
When reporting the location for a linefeed, there is two reasonable way to do so, the first is to report {start: {line: m, column: n}, end: {line: m, column: n+1}}. This approach does not work in VSCode. The second way is to report {start: {line: m, column: n}, end: {line: m+1, column: 0}}. This does work in VSCode, except for when that start location is the location of EOF. In that case I chose to not report end location.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Oct 5, 2019
Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! This approach seems fine to me, but let’s see what other have to say.

@kaicataldo kaicataldo added enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Oct 8, 2019
Copy link
Member

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

One small change request. Also, could you please add unit tests for the new ast-utils method?

lib/rules/utils/ast-utils.js Outdated Show resolved Hide resolved
Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

Note that we still need two more team members to 👍 this PR before we can accept.

@platinumazure
Copy link
Member

@kaicataldo Until the linked issue is accepted, would you like to champion this PR's proposal?

@kaicataldo
Copy link
Member

Yes, I’ll champion this!

@kaicataldo kaicataldo self-assigned this Nov 17, 2019
@kaicataldo kaicataldo merged commit 41a78fd into eslint:master Nov 18, 2019
@kaicataldo
Copy link
Member

LGTM. Thanks for contributing!

@kaicataldo kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Nov 18, 2019
@golopot golopot deleted the location-dangle-comma branch March 1, 2020 15:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants