bug#57884: [PATCH] Flymake backend using the shellcheck program

[Augusto Stoffel <arstoffel@gmail.com>]

On Sun, 18 Sep 2022 at 12:50, Philip Kaludercic wrote:

>>> I don't use `named-let' that much, but calling both the result and the
>>> recursive function `dialect' seems confusing to me.
>>>
>>
>> Welcome to Lisp-2...
>
> It is not so much that they share the same name, but that I don't
> understand why you close to give the function the name "dialect".  From
> what I have seen, named-let is usually given a self-referential call
> like "next", "recur", "loop", etc.

Yeah, I have no strong opinion about the name.

>>> Wouldn't it be cleaner to pull this lambda out into a separate named function?
>>>
>>
>> That's not possible, it needs to be in that lexical scope.
>
> Not if you use `process-put' and `process-get'.  I recently reworked
> flymake-proselint and did the same thing.  IMO it doesn't look that bad:
>
> https://git.sr.ht/~manuel-uberti/flymake-proselint/commit/30c4baa08db32e73d956c978c81a9f79062c2e1d

Honestly, I much prefer to have some state localized in a closure than
in what is effective a global variable...

I like in your backend that you read a JSON output, which presumably
provides the start _and end_ of the diagnostic region.  How did you
convert from line/column to buffer position?

I think it would be nice to extend flymake-diag-region to have signature

   (flymake-diag-region BUFFER LINE &optional COL LINE-END COL-END)

If you provide LINE-END and COL-END, then those are converted to buffer
positions and there's no guessing as to what they should be.

>>> Also what happens if someone doesn't have shellcheck installed?
>>
>> Then Flymake logs a warning and disables the backend. The error message
>> will be whatever make-process gives, which I find more than good enough.
>
> But isn't that rather late to detect the error.  Couldn't you also check
> if "shellcheck" (or whatever the new option name will be) can be found
> in the path before adding the hook in `shell-mode'?

I think this is worse than not doing anything.  If you don't check and
let the backend fail, it gets displayed as a disabled backend, and the
user has the chance to look at the log buffer and try to figure out why
it's not working.  If we did what we suggest, we'd have to reinvent the
wheel to convey that information to the user, I think.