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

Adding end-position in reports #11176

Closed
rocky opened this issue Dec 10, 2018 · 5 comments
Closed

Adding end-position in reports #11176

rocky opened this issue Dec 10, 2018 · 5 comments
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion auto closed The bot closed this issue core Relates to ESLint's core APIs and features 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

Comments

@rocky
Copy link

rocky commented Dec 10, 2018

The version of ESLint you are using.

github master 83017a3

The problem you want to solve.

More detail in location in various report formats.

Your take on the correct solution to problem.

I note that eslint internally records the end positions of messages with endLine and endColumn. However this information does not appear in any of the report styles.

Are you willing to submit a pull request to implement this change?

Sure, I thought would be to start with "stylish" since that seems to be the default. My main concern before undertaking work would be to understand what's acceptable. Is extending the location field in "stylish" acceptable? If not, in what styles would it be acceptable?

There are several ways to indicate a range. As mentioned above, internally there seems to be end line and end column. Since most errors don't span several lines, it would save space in those cases to omit the end line when it is the same as the beginning line.

However something I personally prefer is to give a length, and then there never is a need for a fourth value. Unfortuantely, this kind of information is not available internally. I think it would great if an optional length field were added.

The next question would be how to represent this in say "stylish". Right now it is a pair of numbers separated by colons. For example 5:3 means line 5 column 3. I would propose that 5:3-6 would mean indicate going to column 6 on the same line, and 5:3-6:10 would go from line 5, column 3 to line 6 column 10. And 5:3.3 could mean line 5 column 3 for a length of 3 characters.

That said, it would be nice if there happens to be preexisting art and conventions to use those conventions. I am just not aware of them at present.

@rocky rocky added core Relates to ESLint's core APIs and features 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 labels Dec 10, 2018
@nzakas
Copy link
Member

nzakas commented Dec 13, 2018

Thanks for the suggestion. The reason we don't output this information in any formatters is because we don't always have end position information. We try to add that information if possible, but it's not always possible.

I'm not sure it makes sense to alter the built-in formatters to add this information when it's available, but you could certainly make your own formatter if you feel this information is important for your project.

@rocky
Copy link
Author

rocky commented Dec 13, 2018

I'm not sure it makes sense to alter the built-in formatters to add this information when it's available

I don't understand why it doesn't make sense to alter the built-in formatters to add this information when it is available. Please explain.

@nzakas
Copy link
Member

nzakas commented Dec 14, 2018

Sure! To start, changing default formatters is a breaking change (because we can't know if people are parsing the output), so we need to make sure there's enough value to end users to be worth breaking a part of the experience. In this case, I don't think there's enough value to justify that.

Also, because ending position is not available on all messages, we are left with a situation where we'd need to figure out how to make the formatter output something pretty when there is a mix of messages where some have ending position information and some don't. In the stylish formatter, which is lined up pretty precisely, that would force us to either input spaces or use some other convention to get things to align, and I'm not sure that would be a good user experience.

I'm still of the opinion that your best path forward is to write a custom formatter for your project.

@rocky
Copy link
Author

rocky commented Dec 14, 2018

Thanks for the clarification. I think we are narrowing in on what the concerns are. But there is still a bit of fuzziness here that I'd appreciate understanding. From this I think we can make a more informed decision on ways forward.

First, I should say that a little bit about my use. I am working on tools totally outside of nodejs linters. The only similarity is that the tool reports warnings and errors. Rather than invent yet another report format and write code for that, I took a quick look to see what currently was available. Since my tool is written using nodejs and uses eslint for its linting, looking at eslint was an obvious candidate.

changing default formatters is a breaking change (because we can't know if people are parsing the output

Ok, this makes a lot of sense. Well at least for the "stylish" format which is the default.

For others, not so much. First I am not sure to what extent each in fact conforms to what they purport to be. I'd find it very hard to believe that whatever the visualstudio format that is being mimicked, it doesn't have a provision indicating a range. The tap format strikes me as weird in the way it does nesting and the 1..1 at the top which probably should be 1..n where n is the number of errors (or should it also include warnings?). And to make the 1..1 work the messages after the first one seems artificially nested.

But that is beside the point for the aspect I'm interested in. The larger point, however, remains that I doubt these things follow what the purport to be all that closely in the first place (and to be fair, maybe someof them just can't totally), or that these formats don't already have a provision for indicating ranges.

For TAP, since it is YAML-like, I'd find it hard to believe that adding additional YAML fields in a message would mess up things that parse this. Possibly this, visual studio, and unix, already have some provision for indicating a range. I haven't checked, but I probably should.

Therefore for those formats where there is either already provision for indicating ranges or where adding a range would be unambiguous, tools that parse these might be better served by providing the more detailed information.

But let's come back to "stylish", since that's what I focused on above. If the concern is breaking things that might parse this, an "extended stylish" could be added. In those cases where ending locations are not given, it would be 100% compatible with "stylish". Since that wouldn't be the default it would allow users and tool writers a migration path.

Let me close with the various options I see:

  • Add an "extended stylish"
  • Same as above but just keep it private
  • Start with the eslint routines, but revise more of the internals. (For me, internally a location is: start item, length, container)
  • Look around for another package is perhaps more focused on reporting and allows for more detailed reporting

@eslint-deprecated
Copy link

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jul 14, 2019
@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 Jul 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion auto closed The bot closed this issue core Relates to ESLint's core APIs and features 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
Projects
None yet
Development

No branches or pull requests

2 participants