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 space-infix-ops #12324

Merged
merged 1 commit into from Nov 1, 2019

Conversation

@golopot
Copy link
Contributor

golopot commented Sep 27, 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)

Make space-infix-ops report end location.

const a = p&&q
//         ~   before
//         ~~  after

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

No

@eslint eslint bot added the triage label Sep 27, 2019
@golopot
Copy link
Contributor Author

golopot commented Sep 27, 2019

One thing confuses me. In the docs (https://eslint.org/docs/developer-guide/working-with-rules#working-with-rules) the location column is said to be 0-based, but in test cases the location column is 1-based. What's wrong?

Copy link
Member

g-plane left a comment

LGTM, thanks!

@kaicataldo
Copy link
Member

kaicataldo commented Sep 28, 2019

@golopot I had to look this over myself, but it looks like when it's reported in the rule it's expected to be 0-based, but the error in the results is 1-based (see here). Maybe someone on the team who has more context can speak to why this works that way.

Copy link
Member

kaicataldo left a comment

LGTM, thanks!

@golopot
Copy link
Contributor Author

golopot commented Sep 28, 2019

I guess one reason context.reports expects 0-based column is that it allows passing a token location with context.report({loc: someToken.loc}), and that token columns are 0-based.

And one reason that problems.column is 1-based is that it is a more human friendly format, for example most editors display 1-based columns.

Copy link
Member

mdjermanovic left a comment

LGTM, thanks!

Two more 👍 to accept this.

@kaicataldo
Copy link
Member

kaicataldo commented Nov 1, 2019

Given that we have a champion and three approvals from the team, I'm marking this as accepted.

@kaicataldo kaicataldo added accepted and removed evaluating labels Nov 1, 2019
@kaicataldo kaicataldo merged commit 7e41355 into eslint:master Nov 1, 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 Commit message follows guidelines
Details
continuous-integration Build #20190927.13 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:space-infix-ops-location branch Mar 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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