mohkale / flymake-collection

Collection of checkers for flymake
MIT License
68 stars 13 forks source link

Docs Request: Usage with Eglot #20

Open denisw opened 1 year ago

denisw commented 1 year ago

Thank you for creating flymake-collection! I use it together with Eglot to fill the gaps left by the LSP server (e.g., running ESLint along typescript-language-server). However, doing so is a little tricky as Eglot replaces all registered Flymake backends when activated rather than replacing them.

It would be great if the README had a section on how to work around this. For reference, here is what I do, though I'm not sure it's the best approach:

(defun my/eglot-managed-mode-hook ()
  ;; Add the configured flymake-collection checkers on top of
  ;; eglot-flymake-backend.
  (let* ((entry (assoc major-mode flymake-collection-config))
         (checkers (cdr entry)))
    (dolist (c checkers)
      (push c flymake-diagnostic-functions))))

(add-hook 'eglot-managed-mode 'my/eglot/managed-mode-hook)

emacs-managed-mode-hook is described in the Eglot manual.

mohkale commented 1 year ago

@denisw

I don't think it makes sense to maintain workarounds for behavior set by another package in this one, although I don't mind adding documentation for it to the README if someone creates a PR for it.

meain commented 1 year ago

I think all you need to do is to ask eglot to stay away from completely taking over flymake and just add it as another item.

(add-to-list 'eglot-stay-out-of 'flymake)
(add-hook 'flymake-diagnostic-functions 'eglot-flymake-backend)
peterhoeg commented 1 year ago

The original issue is still valid though and while it stricly speaking may be off-topic here, I think at least figuring out the official way to do things would be helpful.

This is basically the different scenarios:

  1. the language server is good so you don't want any other checks run through flymake. This is the default scenario with eglot that assumes that to be the case.
  2. you want to use both elgot and other flymake checks. Here you need 'eglot-stay-out-of and then you can mess around with flymake-diagnostic-functions manually either as suggested here or through make-local-variable.
  3. there is no LS for a given major mode and as a consequence it's all about flymake
  4. but then we have another case - sometimes you want to use eglot plus zero or more flymake checks or some flymake checks

The example for item 4 is crystal where the language server is still in early stages, so using it alone is not enough, but depending on the type of file you're working with you either want eglot (because it's part of a proper crystal project) or you want a standalone flymake check using the crystal compiler because it's a standalone file.

And I must admit I'm having a bit of a hard time modelling exactly how to deal with the scenario outlined in item 4.

What makes it even worse is of course that you could argue this should be the responsibility of flymake or eglot to know when to enable certain checks and not really flymake-collection.

On a final note - thanks for flymake-collection, it's so easy to add new checks, thank you!!

mohkale commented 1 year ago

@peterhoeg

To be clear, what you're describing is a very specialized workflow configuration and I don't think that belongs in flymake-collection. I'm generally of the mindset that language-server > flymake-checker and actually prefer eglot overriding flymake and flymake-collection. In situations where this is not desired I think it's fine to let users come up with their own way to setup flymake-diagnostic-functions to achieve what they prefer. Trying to cater to every possible combination of language-server and flymake checker is challenging and I don't think will ever be robust enough for everyone. I'm willing to accept patches to improve compatibility here within reason but still think this is a very specific desire that doesn't belong in a generic package like this one.

peterhoeg commented 1 year ago

Right, so I don’t think that I’m the only one with this issue - just as well as some will have multiple flymake checkers running, some would want them in addition to lsp, but I fully understand where you are coming from.

You can also rightfully argue that helpers to assist with this belong in flymake proper or eglot. Or maybe completely outside so that it becomes either the responsibility of the individual or whichever emacs distribution (in my case doom) people are using.

Any plans to upstream any of this into flymake?

mohkale commented 1 year ago

Any plans to upstream any of this into flymake?

There was some discussion of it while considering adding this package to ELPA/Non-GNU-Elpa, the main blocker at this point is just cleaning up the code base and me going through the process for submission (it's on my TODO list, just not a high priority). See also #2.

For reference this package isn't the only one providing a flymake helper. It's actually very heavily adapted from flymake-quickdef.

peterhoeg commented 1 year ago

I had a look at quickdef first but since you have already done the work and created a number of definitions of which I am now using 3, I thought it would make more sense to stick to yours. Very nice to hear that the end-goal is possible upstreaming! Thanks again for your hard work. Emacs 29 with flymake is proving to be very smooth due to amongst other things flymake-collection!