Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use replace-buffer-contents to reformat buffer #124

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BlankSpruce
Copy link

According to replace-buffer-contents documentation this function
replaces buffer contents in smarter way. Especially interesting part:

As far as possible the replacement is non-destructive,
i.e. existing buffer contents, markers, properties, and overlays
in the current buffer stay intact.

One example where some properties are preserved is when origami-mode
is used on the buffer to collapse some regions. After this change
collapsed regions stay collapsed after reformatting

@BlankSpruce
Copy link
Author

I'm not user of vimish-fold so it should be taken with a grain of salt but after a few quick tests it seems that this way to reformat buffer also preserves folds from that module which would close the issue raised in #98.

@twmr
Copy link

twmr commented Feb 20, 2021

FYI https://github.com/purcell/reformatter.el/blob/master/reformatter.el#L294

@lassik
Copy link
Owner

lassik commented Feb 20, 2021

@BlankSpruce Thanks. Good to know that this improves origami and possibly vimish-fold.

The PR linked by @Thisch points out that replace-buffer-contents is not problem-free either, so its use should be optional. We can add a hidden variable to format-all to control whether or not that function is used. But I asked the maintainer of reformatter.el if he'd like to work out a solution that both reformatter and format-all can use. A handful of formatters can also output RCS patches, and there's ELisp code to apply those patches directly.

@BlankSpruce
Copy link
Author

BlankSpruce commented Feb 20, 2021

@Thisch Thanks for pointing that out.

I've just checked how insert-file-contents behaves with origami folds in place and IMO it'd be worse than removing all the folds.
Example with C++ and clang-format:
image

Folded:
image

After reformatting:
image

And what's worse this new fold can't reopened. So I guess insert-file-contents is also not problem-free.

The current solution at least doesn't bring any surprises in contrast to insert-file-contents and replace-buffer-contents.

User might want to preserve some properties of the buffer after
reformatting which can be done by customizing
`format-all-reformat-buffer-function`.
@BlankSpruce
Copy link
Author

I've pushed another proposal. How about letting user decide how they want their buffer to be reformatted? By default it will be the current, fast implementation but they might either choose some other function defined in format-all.el or even provide their own function - I'd probably use custom variant of format-all-non-destructive-reformat-buffer but with some small timeout instead of no timeout.

@lassik
Copy link
Owner

lassik commented Feb 21, 2021

I agree that users should have a choice since there is no one obvious right way to do it. But users of packages like reformatter and many others will be facing essentially the same choice so it would be good to have one package where to make that choice. Let's wait for @purcell's opinion and maybe we can work something out that solves everyone's problem.

@lassik
Copy link
Owner

lassik commented Feb 24, 2021

Just noticed this package by @raxod502 which takes a very interesting approach: https://github.com/raxod502/apheleia

@BlankSpruce
Copy link
Author

Just for the comparison I've extended apheleia locally to support clang-format: it actually works a bit better. Some origami folds might still get broken:

image

The first fold (line 2) is openable but the second one (line 9) isn't.

@lassik
Copy link
Owner

lassik commented Feb 26, 2021

@BlankSpruce Thanks for looking into it in detail.

We should ask origami's author for advice; he is likely an expert on this stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants