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

[Philip Kaludercic <philipk@posteo.net>]

Augusto Stoffel <arstoffel@gmail.com> writes:

> With the attached patch I believe I've addressed all comments from this
> and previous messages.  (In case I missed some detail, the committer
> should feel free to make any desired copyediting.)
>
>
>
> On Sun, 18 Sep 2022 at 11:55, Philip Kaludercic wrote:
>
>> Augusto Stoffel <arstoffel@gmail.com> writes:
>>
>>>
>>> I could split this into two defcustoms if you feel strongly about it,
>>> but it seems a bit of overengineering to me.  Not many people will want
>>>to customize the program name, and the switches would have to be
>>> provided as a list anyway, since we're not going to call this through a
>>> shell.
>>
>> I think it would be good, because then you could make the flags file
>> local in case you need something special, without having to worry about
>> some evil file that sets the command to
>>
>>      '("rm" "-rf" "--no-preserve-root" "/")
>>
>
> Well, fine then.  I've split this into two variables.
>
>> 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.

>>> +         (pattern "^-:\\([0-9]+\\):\\([0-9]+\\): \\([^:]+\\): \\(.*\\)$")
>>
>> Do you think that that using `rx' would make this pattern more maintainable?
>>
>
> I use rx often, but I think this is still at a level where rx doesn't
> really help much.

OK.

>>> +         (sentinel
>>> +          (lambda (proc _event)
>>
>> 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

>>> +    (setq sh--shellcheck-process
>>> +          (make-process
>>> +           :name "luacheck" :noquery t :connection-type 'pipe
>>                     ^
>>                     Typo?
>>
>>> +           :buffer (generate-new-buffer " *flymake-luacheck*")
>>                                                       ^
>>                                                       same here
>
> Ouch, fixed.

1+

>> 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'?