purcell / emacs-reformatter

Define commands which run reformatters on the current Emacs buffer
265 stars 21 forks source link

Customize where to display the error output #3

Open ludwigpacifici opened 5 years ago

ludwigpacifici commented 5 years ago

Hello @purcell

Thanks for this library. This is very useful!

I started to use it with OCamlFormat. You can find the proof of concept here. My end goal is to have an implementation that will be a drop-in replacement of the current Elisp integration of OCamlFormat. On my side, it looks good, except, there is a missing feature: the choice where to display error output.

The existing OCamlFormat integration comes with an option ocamlformat-show-errors:

Where to display ocamlformat error output. It can either be displayed in the compilation buffer, in the echo area, or not at all...

I wonder if there is a way to replicate that via reformatter.el. Would it make sense to add a new argument to reformatter-define to customize where to display the error output?

purcell commented 5 years ago

Thanks for this.

I wonder if there is a way to replicate that via reformatter.el. Would it make sense to add a new argument to reformatter-define to customize where to display the error output?

I don't think that's exactly what you'd need. Instead reformatter-define would need to define (and honour) a variable like ocamlformat-show-errors. I'll have to think about how best to do that because I have a couple of concerns:

ludwigpacifici commented 5 years ago

I believe, the default should be the current reformatter.el behavior (on error, echo area says which buffer will contains errors and on success, no unnecessary output):

reformatters would lose some consistency, which is a goal of reformatter.el.

Agreed. I think there is a limit to be set. It feels similar to use-package ; it makes packages setup homogeneous even though each one of them has its own flavor and non-consistency is handled for example via :after (also mentioned in reformatter.el comments)

ludwigpacifici commented 5 years ago

Looking into more details, reformatter.el uses insert-file-contents and ocamlformat.el uses replace-buffer-contents. Is there a reason to prefer one over the other?

The documentation seems to favor the the use of replace-buffer-contents:

This function attempts to keep point, markers, text properties, and overlays in the current buffer intact [...] this behavior is useful is external code formatting programs

Even though insert-file-contents called with replace not nil (like it is done in reformatter.el) seems to preserve similar properties:

If the argument replace is non-nil [...] This is better than simply deleting the buffer contents and inserting the whole file, because (1) it preserves some marker positions and (2) it puts less data in the undo list.

The documentation is not very clear.

purcell commented 5 years ago

Looking at the C source code for both functions, it indeed looks like replace-buffer-contents might be more rigorous in its efforts to minimise changes. I'll investigate using that instead.

purcell commented 5 years ago

Damn it, I was testing this with Emacs 26.1, and hit exactly the issue you've had to work around. 😆 I don't really want to maintain different code paths, so I think I'm going to stick with insert-file-contents for now. It has been used in practice for several formatters for quite a long time, and I'm not convinced that anyone would ever notice any potential differences relative to replace-buffer-contents.

ludwigpacifici commented 5 years ago

Apologize, I could have warned you about the bug with replace-buffer-contents with Emacs 26.1since I use it as well.

I agree your argument of simplicity especially if the outcome is limited. That is one reason why I decided to play with reformatter.el for ocamlformat :-)

purcell commented 5 years ago

Haha, I was just unlucky that my current working Emacs version is the one with the bug!

purcell commented 5 years ago

Currently reformatter.el defines commands which only display the errors when called with a prefix arg. I feel like that choice might already be at odds with how ocamlformat currently works, regardless of the method used to display the errors?

ludwigpacifici commented 5 years ago

I am not sure to understand you. Do you mind to rephrase your question?

purcell commented 5 years ago

Sorry, you're right - that wasn't very clear! Okay, so if you used reformatter to make an ocamlformat reformatter, the resulting ocamlformat-region command would only display errors if invoked with a prefix argument. So even if it could be configured to display errors in different ways, you wouldn't ever see them unless you asked to (with that prefix argument). Does the existing ocamlformat command work like that, or does it unconditionally show errors using the configured method?

ludwigpacifici commented 5 years ago

Thank you for the additional information :-)

ocamlformat-region command would only display errors if invoked with a prefix argument.

I have not noticed that. After some experimenting, it seems the prefix argument display-errors cannot be set from the macro reformatter-define (correct me if I am wrong). However, C-u M-x ocamlformat-region will set it and *ocamlformat error* buffer will show up.

What is the reason to use prefix arguments instead of passing arguments via the macro reformatter-define?

As far as I understand, here is the behavior comparison between ocamlformat-show-errors vs refomartter.el with display-errors:

Does the existing ocamlformat command work like that, or does it unconditionally show errors using the configured method?

ocamlformat unconditionally shows errors on stderr (@jberdine correct me if I am wrong). Do you think something needs to change here?

failable commented 3 years ago

Can I just message the error output instead of popup and guiding me to see the error buffer?