Implement find_errors and return LintMatches directly

[FichteFoll]

Fixes #33

Instead of creating and then parsing a string with a regular expression,
we can instead build and return a list of LintMatches (a typed dict)
directly and skip some indirection.

I am not sure if lint is supposed to be overridden like this, but I
do remember advocating for it a couple years ago when the change for a
proper data structure for lint results was discussed. It just now
happened to be a convenient way to implement end markers for the
highlighted region. (@kaste The implementation, the point to patch, has
changed, lint is considered private)

In this particular instance, it allows us to specify a full region for
marking the error, where we now specify the entire match including the
remainder of the comment instead of just the trigger word. This can be
changed, of course, but I figured it wouldn't be a bad idea initially.

---

The motivation for this change was to allow for the trailing ! mentioned in #34 to be marked, since it isn't a word character and thus wouldn't be picked up by the automatic region expansion from the marked character.

Before (#34)

After (with #34)

[FichteFoll]

I wonder whether it would be reasonable to add this use case to the default configuration, even though it is specific to only rust. What do you think?

[kaste]

Implementing lint is problematic. I think it should be considered private as it also checks view_has_changed? and runs filter_errors.

Ideally you would override parse_output, which also outputs an iterable of LintError. I think at this state maybe the public version of LintError is actually LintMatch. For example offending_text must equal view.substr(region) which it doesn't here if I read this correctly. It is of course just an optimization; we actually ask our virtual view for the text because it's faster than view.substr. It also has all this other stuff in it, like uuid, a user cannot know and should not control. So the recommended way is to override find_errors actually which yields LintMatches. All JSON linters implement find_errors. (Two years later but #27 (comment) is still correct.)

I actually dislike marking the whole line though. But you do fix #33 here which is nice.

[FichteFoll]

Fine with me. The documentation on LintError was definitely sparse, unlike LintMatch which I just didn't check. I knew we discussed this somewhere, but I certainly didn't remember it was even this repo. I'll move the lint override to parse_output and provide a blanket cmd.

Should I also change the comment_selector to use just selector and then use the virtual views instead of manually handling the regions? That will have the side effect of not showing "annotations" in the status bar, but that may even be desirable since it's basically always active anyway. It will also use the same settings as other linters.

I've been using the marks for the entire line until now and I grew rather fond of it. If you don't think it should be the default, I could and an option for it, but I believe marking the entire line is just more useful, because it makes it stand out more in code and in the minimap.

[kaste]

Should I also change the comment_selector to use just selector and then use the virtual views instead of manually handling the regions?

That would be a mistake. If you use "comment" as selector, each view would be split in dozens of "lint-tasks". I don't think that overhead is worth it.

In SublimeLinter land we have a fan-base for minimal UI so not underlining the whole line is probably a must. (No, really, we get more complaints about too prominent UI elements than the other way around.) FWIW, when you don't yield a column (just the line) SL will actually highlight the complete line if no_column_highlights_line: true is set in the SL settings. I actually don't use this setting but we have code for it. Visibility in the minimap (which I don't use either) should also be handled in SL core if possible.

[FichteFoll]

I think that should do it. Unfortunately I introduced a merge conflict with #34 because I added another setting for marking the remainder (the "message") of the comment line.

[kaste]

This did initially not work because it was mixed with the other PR. (You used view.find_by_selector(self.settings['comment_selector']).)

I switched to find_errors because that's the endpoint the other linters overwrite as well. (To actually use the virtual view would be thread safe but is out of scope here.) By doing so we emit LintMatches which is part of the official API whereby LintError is not - admitted it has the better name. We shouldn't have two versions of the same data as "official" API.

I also did some refactoring on the go. There is still the merge conflict with #34.

[kaste]

Why is travis running here? I thought it is a paid service now? Do you have a paid account, @FichteFoll? I don't.

[FichteFoll]

LGTM. Rebased onto master and made small changes on top.

Regarding Travis, I have an educational "PRO" account but I don't quite remember if that comes with perks for travis, especially since their change is rather recent.

[kaste]

The "superfluous" accumulate function is actually just a backport as the initial argument is available for python 3.8+. (I think it is more readable because typically programmers don't think of using chain here but rather implement it like (_ + some_offset for _ in accumulate(it)) t.i. shift all values right or left.)

I don't know how you rebased here but you took over the authorship of "26a5ea0 (Simplify error_type detection)". In case you used GitSavvy that would indicate a bug I would like to know about. 😁

[FichteFoll]

I edited the commit and replaced _ with it because _ is, by convention, a name that should only be used if the value is to be discarded, which wasn't the case here. Sublime Merge (rightfully) changed the author then, because I made some changes and I didn't bother going back to change the author or adda Co-Authored-By line.

[kaste]

Ok, interesting. In GitSavvy you would press E to edit a commit. If you then follow the instructions you get (by default) the typical split: author of the commit remains but the editor gets the committed credit. This is the default of git. I think it is the correct default as changing a variable name usually does not change the authorship; ("Schöpfungshöhe"). (I remember this so well because I fixed a bug around this in GitSavvy. So I have a spotty eyesight currently.)