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

[Philip Kaludercic <philipk@posteo.net>]

Augusto Stoffel <arstoffel@gmail.com> writes:

> 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...

Not quite, you can only access the value if you have access to the
process object.  I guess you can argue that the process object is stored
as a file local variable which sort of makes it global...

> 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 didn't do that, Flymake takes care of that if you give it the buffer
position (unless I misunderstood something, but it appears to be working
for me).

> 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.

On the other hand there might be people who don't care about shellcheck
(say they just open a shell script once a year) and any error message
would just be an annoyance.

The most important part is having the ability to use shellcheck
prominently documented, the rest is probably just a matter of taste.