[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.
From: |
Theodor Thornhill |
Subject: |
bug#50244: 28.0.50; Support project-wide diagnostics reports in flymake.el |
Date: |
Sat, 23 Oct 2021 22:50:00 +0200 |
João Távora <joaotavora@gmail.com> writes:
> On Sat, Oct 23, 2021 at 8:22 PM Theodor Thornhill <theo@thornhill.no> wrote:
>> (sent this mail some days ago, but the bug was archived, so not sure if
>> it worked. Trying again. If this is only noise, then I'm sorry, but
>> hopefully ok considering i found a typo in the old patch :))
>
> No problem, and yes I got the other one. It's just I've been again
> ompletely utterly busy so it'll be a while. I took the Flymake thing out
> of my brain cache and it's going to take a while to get it back.
>
Didn't mean to rush things, we do have time :)
>> I finally did get to test this out a little, and I seem to have hit some
>> small snags I can't really understand. I'd love some pointers as to how
>> to continue!
>> (the patch I currently use is attached at the bottom)
>> Ok, so what happens is:
>>
>> - There is no backend set on the list-only-diagnostics as for now, and I
>> cannot for the life of me see how to set them, as it doesn't look like
>> eglot itself sets it. Is there some reflection going on here that I
>> have missed?
>
> No, I don't think so. Maybe I simply decided that no backend was
> necessary? Is it? If it is, we might to add some mechanism so
> that it is assigned.
>
I don't think a backend really is necessary anymore. I've been checking
with more servers lately, and it seems to work better with some than
with others. And especially elmls seems a little wonky here.
>> - The function in question is riddled with fallbacks and failure
>> checking. Are we sure we need all that? I see most of the commits
>> relating to the range checking is from 2018, so they may not be
>> relevant anymore?
>
> I'm not sure I follow here. I wouldn't delete any checks until I'm
> sure they are doing nothing. 2018 or not, I do remember a lot of
> servers providing very strange diagnostics and invalid ranges.
>
Yeah, I didn't remove any checks yet, and likely will not. I'm merely
guessing that the lsp scene has settled a bit lately.
>> - The diff as supplied appends the list-only-diagnostics just fine to
>> the flymake buffer, but I believe since the backend isn't set, eglot
>> doesn't report the proper diagnostics when I visit the file. I have
>> to make a bogus edit to trigger the eglot-flymake-backend function.
>> (This could just as well be some unrelated eglot/elmls issue...)
>
> Ah, this rings a bell. You're talking about visiting a file by clicking a
> "list-only diagnostic" in the list, right? And then you wnat Flymake to
> resume checking and replace them with "real" ones. Yes, might be
> eglot/elmls issue, but usually there's not much we can do: some servers
> are like that.
>
Yes, you are probably correct here.
> To keep things consistent we could remove the relevant list-only
> diagnostics and re-report them via the normal mechanism if the server
> is taking too long.
>
I'll look into this, and see if I can find a solution for this.
> Do you understand this idea? The user wouldn't, in principle, be able to
> tell that the freshly annotated diagnostics weren't collected just now
> when he visited the buffer, but much much earlier. The project-wide
> listing should, in principle, also remain consistent.
>
> ...though probably unsorted. If I remember correctly consistent sorting
> is still a problem.
>
Yes sorting is a little off, and I'm looking into that as well. I think
we need to sort on filename, line and col in that order to get it
correct, right?
>> - What would be the best way to get the proper line and column? Now I
>> simply use the range, but considering there's a lot of error handling
>> there I guess that is a little risky? I did have a version earlier
>> using with-temp-buffer and finding the correct positions, but that
>> seemed more complicated than needed.
>
> Risky for what? I don't follow
>
I'm thinking that if I avoid doing the same error handling as in the
normal case (which requires a live buffer), chances are that the range
may be wrong. But since I didn't yet see such a case, I'm thinking some
of the error handling is redundant. I don't have enough proof yet to
remove them, but there may be some problems that I didn't yet discover
with the patch I sent.
> If you used with-temp-buffer, I guess you visited the file. That would
> go against the idea of list-only-diagnostics, which are supposed
> to be for unvisited files (if the file were visited you'd have Eglot/Flymake
> actually checking). So using the LSP range and converting it to
> flymake-make-diagnostic's protocol seems ok.
>
Great! Yes, I didn't want to visit the buffer, but that seemed as a good
way to go at the time, considering that it at least won't clutter the
bufferlist with every file in a project on load :)
>> I'd love to get your thoughts on this :)
>
> Hope I could help and sorry if the thoughts were a little sparse.
> As I said, this is out of my cache and will be for some time.
>
No, this is great. It confirmes some of my hunches, so that is good at
least.
> Do consider patches/changes to flymake.el as well. This new
> mechanism is very new, it's possible it needs some tweaks.
>
I think most of what _I_ need is covered, in some form or other. I'll
prepare a patchset for both eglot and maybe flymake sometime soon, and
you can look at it when you have some free time and energy.
Theo