Gutter highlights only show on first line

[dertieran]

This might not be a problem with the linter UI but with the linter/plugin itself, but I still wanted to ask here first.
I'm using linter-eslint and noticed some weird underline behavior for some rules.
(The once I noticed are mainly from the eslint-plugin-promise so it could be related to that)

The screenshot below shows just one warning and the gutter seems to be right,
but the underline is all over the place which is quite annoying for readability.

[steelbrain]

That's actually bad behavior of the ESLint plugin, instead of underlining just the start, it underlines the entire callback function so it ends up looking like it does

[Arcanemagus]

Actually, this is the combination of a few things:

• Atom bugs out with large decorations on text and only intermittently shows them, that's why some lines are underlined while others aren't.
• eslint-plugin-promise looks to have a potential bug as it's reporting the entire program node as the location of the issue, this causes ESLint (as of v4) to generate a range over the entire function. This isn't a linter-eslint issue since it is just doing what ESLint says to do, and ESLint is just doing what eslint-plugin-promise is telling it to do. @dertieran You probably should file an issue on that plugin to see if they can narrow the range they report down to a more targeted location.
• @steelbrain as this is a multi-line range should linter-ui-default be marking the gutter for each line in the range or just the start of the range as it's doing here?

[steelbrain]

@Arcanemagus Good question, multi line ranges this long aren't all that common, but yeah it should mark gutter for all lines

[dertieran]

Thanks for the response I expected it to be the plugin.
I might open an issue there and/or wait for #285

[xjamundx]

👋 author of eslint-plugin-promise here. I'm a bit rusty on my eslint skills. Can anyone point out in eslint-community/eslint-plugin-promise#66 what I should do differently? Basically warn about a more exact node or something?

[Arcanemagus]

@xjamundx Responded with my best guess over there 😉.

[j-cong]

@steelbrain I have same underline behavior problem even without additional plugins enabled, with rules like max-statements, class-methods-use-this, prefer-arrow-callback.
I have tried to install previous versions of eslint (which I have globally installed), and for me this problem appears starting from [email protected]. I'm using version [email protected] for now , that doesn't cause such undesirable behavior.
I think that an option to disable only underlining and remain displaying issues in gutter could also be useful in such cases.

[pvdlg]

The same issue happen with the rule require-jsdoc that is part of eslint core.
I don't know if this new behavior of eslint is intentional or not.

[Arcanemagus]

@vanduynslagerp The new behavior is definitely intentional on ESLint's part, the issue is that linter-eslint can't easily "filter it out" as there are as many edge cases as there are files to lint 😞.

I'm guessing that rule is returning the function node, which is leading to that entire function being highlighted, you could try coming up with a better range to highlight and proposing it on the ESLint repo, but that already sounds like the best option for that so I'd just consider that extra motivation to fix the rule 😛.

[mxchange]

I have a similar issue with linter-tslint.

[Arcanemagus]

Filed a bug on Atom about this decoration issue, hopefully once that gets fixed it shouldn't look so bizarre.

The gutter not highlighting on each line is still a bug that should be fixed here so I'm updating the labels appropriately.

[mxchange]

I think this is related: #381

[Arcanemagus]

#381 has nothing to do with this, this is half a bug in Atom with how it displays large range highlights, and half a bug with how linter-ui-default highlights the range in the gutter.

[Arcanemagus]

Turns out the bizarre highlighting was actually a side effect of the hacky method used to highlight lines here. I've put up #431 in order to update to the new text decoration style which fixes that. The gutter icons still need fixing though.

[lydell]

I really like that errors are underlined. I do bump into the issue of very large areas being underlined, though, especially when using linter-eslint.

• That is not very readable.
• It is especially problematic when using "Tooltip Follows: Keyboard". Once you've entered the underlined area, there's no way to "escape" from the tooltip other than moving outside the area. This makes it difficult to edit code in there (potentially to correct the error) since the tooltip obscures the code.

Example: The entire file is underlined

This is when using flowtype/require-valid-file-annotation.

Example: A whole block is underlined

This is when using no-constant-condition.

Discussion

• I guess I could just meticulously report issues for every ESLint rule I come across. I get the feeling that ESLint (plugin) developers mostly care about the start location though, since the ESLint CLI only uses that. (I think only editors use the entire range?)
• Would it be nice if errors were only underlined until the first line break? (Potentially behind an option.) I think so. That would improve usability in one swoop.
• The somewhat related "Tooltip Follows: Keyboard" issue I mentioned perhaps deserves its own discussion, but is good to keep in mind when thinking about this.

Thoughts?

[Arcanemagus]

I guess I could just meticulously report issues for every ESLint rule I come across. I get the feeling that ESLint (plugin) developers mostly care about the start location though, since the ESLint CLI only uses that. (I think only editors use the entire range?)

Fascinating, the only CLI formatters that report ranges are the ones meant to be parsed by tools. In any case, highlighting the entire file is what is being requested by the plugin. You can file an issue there and see if they have a tighter range they might be able to report, the error looks like one that should be reported on the entire file though. For a bit more detail on why this can't be easily handled in linter-eslint see AtomLinter/linter-eslint#908. This has nothing to do with linter-ui-default though.

Would it be nice if errors were only underlined until the first line break? (Potentially behind an option.) I think so. That would improve usability in one swoop.

I'm personally very much against this, the provider is giving a range, and the entire range should be marked.

Once you've entered the underlined area, there's no way to "escape" from the tooltip other than moving outside the area. This makes it difficult to edit code in there (potentially to correct the error) since the tooltip obscures the code.

The tooltip should disappear as soon as you start typing, please file a separate issue if that isn't working as it should for you. Also I could definitely see Escape being used to dismiss the tooltip, I actually thought it already did but it doesn't seem that way.

---

With the underline issue fixed in #431, the only remaining part of this issue is the gutter highlights not showing up on each affected line... if they even should? I'm updating the title to reflect that.

[lydell]

the error looks like one that should be reported on the entire file though.

Nah, the first line would be just as correct, in my opinion.

I'm personally very much against this, the provider is giving a range, and the entire range should be marked.

rm -rf /. Glad there's --preserve-root by default.

When a whole file is underlined, or a big part of it, do you personally think that:

• it is kinda OK
• it is kinda annoying

If you think it's kinda annoying, do you think that:

• it's an unfortunate problem that there's no good solution for
• there is a solution but I've seem to have lost what that is?

As far as I understand, ESLint seems to many times report entire AST nodes as locations for errors, which naturally can result in very large ranges. Not even ESLint's own "codeframe" formatter shows the entire ranges, only the first line (and a few surrounding lines for context):

$ eslint whole-file.js --format codeframe
error: Flow file annotation is missing (flowtype/require-valid-file-annotation) at whole-file.js:1:1:
> 1 | import { CompositeDisposable } from 'atom'
    | ^
  2 | 
  3 | import Commands from './commands'
  4 | import type { UI, Linter as LinterProvider } from './types'


1 error found.

I think it could be resonable to think of the provider giving a range telling where the erroneous code is and asking for that to be conveyed to the user in a helpful way. In other words, the provider is only saying what it knows not what it wants, and leaves it up to the UI package to decide how to display things.

The tooltip should disappear as soon as you start typing

Thanks, it never occurred to me to try that. Probably because I didn't know what to type because I couldn't read :) The tooltip comes back as soon as you start moving again, though. Tricky to say how it should work here.

[Arcanemagus]

Not even ESLint's own "codeframe" formatter shows the entire ranges

Correct, as I said before none of ESLint's CLI reporters meant for human consumption utilize the ranges at all. codeframe is based entirely off the starting point. This bug in the CLI reporters has nothing to do with the discussion here though.

I think it could be resonable to think of the provider giving a range telling where the erroneous code is and asking for that to be conveyed to the user in a helpful way.

That's what is already done: The provider gives a range and the UI conveys that range to the user.

the provider is only saying what it knows not what it wants, and leaves it up to the UI package to decide how to display things.

If you feel that you have a better solution to how to display the information please file an issue or better yet a PR, or create a UI of your own. @mehcode's linter-ui-plus is an excellent example of an alternative take on the panel implementation.

As I already stated though, the best place to fix this particular issue you are bringing up is to do so in the ESLint rule, that way all ways of reporting information to the user, whether that's in Atom, VS Code, ST, the CLI, etc. will show the new, better, range. Your "whole block" example is a perfect place where the range should definitely be constrained to just the part that is triggering the constant condition. The rule shouldn't be reporting the entire if statement's node.

[lydell]

codeframe is based entirely off the starting point. This bug in the CLI reporters has nothing to do with the discussion here though.

I don't consider this a bug in the CLI reporters, but a feature. I think the authors of the reporters realized that the start of the range is by far the most useful, while the rest mostly redundant, so they chose to only show the start. So I do think this is relevant to the discussion: The CLI reporters have set an example that could be followed.

Your "whole block" example is a perfect place where the range should definitely be constrained to just the part that is triggering the constant condition.

True. But what about this one:

That's from the consistent-return rule. Isn't it correct to report the entire return statement here? So even if I did report this to ESLint, they wouldn't "fix" it. So I would still end up with all the underlines.

---

I totally understand if you feel strongly about this and don't want to change/extend linter-ui-default, and that I'm free to fork it if I want to. I just have a hard time letting this discussion go because the reasons not to change anything feel a little ... artificial :) Like (if you exaggerate just a little) if linter-ui-default was made for robots, not humans. Don't get me wrong, I appreciate all the hard work that's been put down in atom-linter, and it's actually really great. This overwhelming underlining thing is just a little stumbling block I run into every now and then :)

If you want to end the discussion here, that's fine. Thank you for taking the time to answer! I'll think about making a fork with the sole change that ranges are clipped to the first newline, and that otherwise tracks upstream very closely.

[Arcanemagus]

I think the authors of the reporters realized that the start of the range is by far the most useful

I'm almost certain the reports haven't been changed since before ESLint even had range reporting, it's not that they chose this way of working, it's just that is what was available when they were designed.

True. But what about this one

I wasn't saying that all examples were things that could be better served by tighter ranges, that's one where it definitely should be the entire second return statement, although I would say just return importEntry(script) should be reported personally. I was just saying that a lot of the ranges that are currently entire nodes could probably be better written as tighter ranges. Remember that this "highlight the entire node" is brand new with ESLint v4, before that change only the first character was reported at the end so rule authors pretty typically were lazy here and just reported the node since it didn't matter. Rule authors are still adjusting to this change and there are many opportunities for better ranges to be reported back.

the reasons not to change anything feel a little ... artificial
Like if linter-ui-default was made for robots, not humans.

I personally like to have as much information as possible, and I like it to be correct, so making the ranges "fuzzy" or altering what is being reported as "truth" from the linter just goes against the "correctness" of things, at least as I see it. 🤖

I've actually found several bugs in linters themselves and reported them back up for all users of the linters to get the fix, all from checking that what the linters are giving is actually a real point in the document, instead of just clipping it down to a valid point.

This overwhelming underlining thing is just a little stumbling block I run into every now and then :)

It bugs me too sometimes when the range really shouldn't be that large, so you aren't alone there.

If you want to end the discussion here, that's fine.

It's always interesting hearing "the other side", in this case it seems you are going for ease of use, which isn't a perspective I take into account as often as it should be.

I appreciate all the hard work that's been put down in atom-linter, and it's actually really great.

@steelbrain is the one who has mainly worked on linter and linter-ui-default, I just work on the providers 😛.

I'll think about making a fork with the sole change that ranges are clipped to the first newline, and that otherwise tracks upstream very closely.

It's likely a fork of linter-eslint will be easier to maintain 😉, especially since that's where most of this issue is, very few other providers report multi-line ranges. It's likely your changes would need to be in this area.

[lydell]

Thanks for all the answers, and the pointer to the relevant linter-eslint code!

I think I'll go down the path of reporting a few ESLint rules for having too broad ranges, and see what kind of responses I get. After some time I'll re-evaluate if I need to contribute to linter-eslint or fork something :)

[lydell]

In case somebody finds this conversation when searching, I ended up creating this hack to make errors singleline: https://atom.io/packages/linter-patch-errors-singleline

[steelbrain]

I think you had a positive idea @lydell @dertieran and others.

The thing is, providers can do this individually, it would be amazing if they did. but Linter would be not as useful if every provider did this. I know some packages that use linter to highlight ALL the code that is not covered by flow. It would be useless if they only underlined one line because you want all the multi lines that are not covered by flow.

But I would be fine by having a single provider like ESLint trim down their ranges to just one line, if that makes sense. and while we're at it, until first character instead of first full line would also be preferrable (but then again ranges won't be the real "ranges" they would be start points so I'll let the current maintainer of linter-eslint decide on that)

[steelbrain]

Closing as external but please feel free to continue discussing, if you have any disagreements to what I said, please feel free to voice them and I'll reopen the issue

[lydell]

It would be useless if they only underlined one line because you want all the multi lines that are not covered by flow.

I suggested an option to only highlight the first line :)

But I would be fine by having a single provider like ESLint trim down their ranges to just one line

And linter-elm-make. And possibly others.

Anyhow, I'm satisfied with my "hack" solution and shouldn't really comment more here :)

[steelbrain]

I would be interested in a function in atom-linter npm package that providers use that contains the logic your hack currently does, so providers can trim the ranges using that logic before they send to Linter. just saying

[Arcanemagus]

The multi-line highlighting part of this was already solved in #431, this issue is still open due to the gutter highlights only showing on the first line of a range. If you think it's fine to leave them as only showing on the first line re-close this @steelbrain 😉.