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

Return range in the lint result #3307

Closed
sindresorhus opened this issue Aug 7, 2015 · 21 comments
Closed

Return range in the lint result #3307

sindresorhus opened this issue Aug 7, 2015 · 21 comments
Assignees
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 backlog core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint

Comments

@sindresorhus
Copy link
Contributor

Currently ESLint returns the line and column when linting. However, with editor plugins it would be useful to get the full range so instead of just visually showing where the violation starts it could mark the whole range of the violation.

For example, here the Atomlinter ESLint plugin could use the range instead of just marking the first character.

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@eslintbot
Copy link

Thanks for the issue! We get a lot of issues, so this message is automatically posted to each one to help you check that you've included all of the information we need to help you.

Reporting a bug? Please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. The source code that caused the problem
  3. The configuration you're using (for the rule or your entire config file)
  4. The actual ESLint output complete with line numbers

Requesting a new rule? Please be sure to include:

  1. The use case for the rule - what is it trying to prevent or flag?
  2. Whether the rule is trying to prevent an error or is purely stylistic
  3. Why you believe this rule is generic enough to be included

Requesting a feature? Please be sure to include:

  1. The problem you want to solve (don't mention the solution)
  2. Your take on the correct solution to problem

Including this information in your issue helps us to triage it and get you a response as quickly as possible.

Thanks!

@mysticatea
Copy link
Member

👍

However, I guess it shows unexpected ranges if semicolons are omitted.
Related: eslint/espree#41

@mysticatea mysticatea added the triage An ESLint team member will look at this issue soon label Aug 7, 2015
@ilyavolodin
Copy link
Member

I think this would be great to add. Unfortunately, it requires manually modifying every single rule we have (or at least most of them).

@nzakas
Copy link
Member

nzakas commented Aug 10, 2015

Yeah, this would be a significant change (need to update every rule, plus change the signature of report()) so I'm not sure if it's realistic to do this. Plus, third-party rules would all have to be updated if you assume every result will have a range.

@sindresorhus if only a handful of rules returned range information, would that be useful to you?

@ilyavolodin
Copy link
Member

@nzakas We could probably modify it so that any time we use context.report and pass it an existing node, we extract both start and end position from the node. That would only affect some of the rules, but it's probably a good start.

@sindresorhus
Copy link
Contributor Author

Plus, third-party rules would all have to be updated if you assume every result will have a range.

You don't have to assume that though. How about adding a range property to rule results incrementally while still preserving line/column. That way tools can first check range and fall back to line/column. Nothing breaks for existing consumers and external rules.

@sindresorhus if only a handful of rules returned range information, would that be useful to you?

Yes, even just a few rules supporting it would improve the UI for editor linters.

@nzakas
Copy link
Member

nzakas commented Aug 11, 2015

OK, I'm not sure we can commit to this at this point. Right now, each message contains the exact same data, and it's hard to tell what effect having different data on different messages would have in the ecosystem.

@ilyavolodin ilyavolodin added enhancement This change enhances an existing feature of ESLint backlog core Relates to ESLint's core APIs and features accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Sep 16, 2015
@ilyavolodin
Copy link
Member

I marked this as backlog, because we can't commit to this right now and put it on the roadmap, but I think ideally, this is something we want to do at some point.

@nzakas
Copy link
Member

nzakas commented Sep 17, 2015

👍

@mysticatea
Copy link
Member

I will work on this.

My plan is that I add lineEnd and columnEnd properties to the result of eslint.verify().
And I will check all core rules.

{
    fatal: false,
    severity: 2,
    ruleId: "semi",
    line: 1,
    column: 23,
    lineEnd: 1,
    columnEnd: 23,
    message: "Expected a semicolon.",
    fix: {
        range: [1, 15],
        text: ";"
    }
}

Those properties are calculated from the arguments of context.report(params) in the following way.

var line, column, lineEnd, columnEnd;

var loc = params.loc || params.node.loc.start;
if (loc.end) {
    line = loc.start.line;
    column = loc.start.column;
    lineEnd = loc.end.line;
    columnEnd = loc.end.column;
} else {
    line = loc.line;
    column = loc.column;
}

For Example:

// Pattern 1
context.report({
    node: node,
    loc: token.loc,       // lineEnd = token.loc.end.line;
    message: "test"
});

// Pattern 2
context.report({
    node: node,
    loc: token.loc.start, // lineEnd is undefined;
    message: "test"
});

// Pattern 3
context.report({
    node: node,           // lineEnd is undefined;
    message: "test"
});

In short, the current code of core rules and plugins don't make lineEnd and columnEnd.
We should update rules with using loc property as {start: {line: number, column: number}, end: {line: number, column: number}}.

This is in order to keep compatibility of editors.
Especially in the case of the pattern 3, if we use node.loc.end, editors might show unexpected many red lines for plugin rules.

@eslint/eslint-team I want to ask opinions.

@ilyavolodin
Copy link
Member

var loc = params.loc || params.node.loc.start;

This wouldn't work if only node is passed in, but I get the general idea. 👍 from me

@nzakas
Copy link
Member

nzakas commented Feb 10, 2016

Sounds good to me, too.

@nzakas
Copy link
Member

nzakas commented Sep 1, 2016

@mysticatea was lineEnd added?

@mysticatea
Copy link
Member

The implement of this issue has done in #6640.
Tweaking of core rules remains.

@Arcanemagus
Copy link

Arcanemagus commented Oct 6, 2016

Have any of the core rules been updated yet?

Thanks to @IanVS's brilliant idea to actually search, it looks like at least no-unreachable was updated 😛.

@mysticatea
Copy link
Member

Yes, only no-unreachable rule uses this feature for now: #6583

@nzakas
Copy link
Member

nzakas commented Oct 7, 2016

@mysticatea if you want to keep this issue open, can you make a list of rules to that need to be addressed?

@mysticatea
Copy link
Member

I will start to work on this within a week.
(I re-checked rules, then I found that several rules are using the range feature.)

@kaicataldo
Copy link
Member

Is this still being worked on?

@mysticatea
Copy link
Member

Yes, I will do. I'm sorry, I'm busy for now. I think I can get time within the next month.

@not-an-aardvark
Copy link
Member

I think this is basically fixed now that b0c63f0 has been merged, so I'm closing this issue.

nostalic added a commit to nostalic/tern-eslint that referenced this issue Aug 23, 2017
* Remove bypass for eslint/eslint#3307
* Update tests to match updated error ranges
* Update browserified eslint to v4.5.0 for codemirror demo page
* Update travis configuration to match eslint ^4.0.0
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 7, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 7, 2018
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 backlog core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
None yet
Development

No branches or pull requests

8 participants