lassik / emacs-format-all-the-code

Auto-format source code in many languages with one command
https://melpa.org/#/format-all
MIT License
604 stars 105 forks source link

Implement "format region" feature for perltidy #233

Closed tsilvap closed 1 year ago

tsilvap commented 1 year ago

Context: perltidy can take a code block in its stdin, and it will return it properly formatted. It doesn't need to be the source code of an entire file.

In this commit, we make a few changes to allow passing in just the active region's code to the formatter, and then replacing the region with the output of the formatter. Then, we use that to enable "format region" for perltidy.

With the changes in this commit, we should also be able to enable "format region" for other formatters, as long as they can accept a code block (not necessarily an entire file's source code) and return that block properly formatted.


Note: this depends on https://github.com/lassik/emacs-format-all-the-code/pull/229 being merged to master first.

lassik commented 1 year ago

Sorry, I'm going to have to decline this one.

Region formatting is intentionally only supported for those formatters that have a special command line flag to do it.

In the general case, it's not reliable to take a region of the buffer, send it to the formatter, and splice it back. The formatter will be lacking information about the context around the region, so it will not detect things like being inside a string literal. More importantly, indentation will be difficult to handle correctly.

Format-all is simple and reliable because I've systematically avoided hacks and workarounds. I've asked upstream formatter authors to solve several problems in the formatter itself, and they have generally been helpful. Please ask the authors of perltidy if they'd be willing to add a region flag. If they do, then format-all and other editor plugins can use the flag.

tsilvap commented 1 year ago

Sorry for the late response. Also, now I realize I should've opened an issue to discuss this first. I'll reply to your comment here, but if you prefer that I open an issue if I want to further discuss this, let me know.


In the general case, it's not reliable to take a region of the buffer, send it to the formatter, and splice it back. The formatter will be lacking information about the context around the region, so it will not detect things like being inside a string literal.

IMO that's not a problem: it's up to the user to mark a reasonable region to send to the formatter. perltidy works fine if you send a single statement, or an if block, or a sub block, and so on. If the user sends an "incomplete" block to perltidy, it'll return an error or spit out some bad formatting, and then the user should just not do that.

More importantly, indentation will be difficult to handle correctly.

Some formatters strip whitespace at the beginning if you send a region and in that case replacing the region wouldn't work well. But perltidy preserves the indentation and it works as expected. My idea was to only use format-all--region-easy with formatters that don't have this indentation problem.

Format-all is simple and reliable because I've systematically avoided hacks and workarounds. I've asked upstream formatter authors to solve several problems in the formatter itself, and they have generally been helpful. Please ask the authors of perltidy if they'd be willing to add a region flag. If they do, then format-all and other editor plugins can use the flag.

I don't think it's fair to say that sending just a region to the formatter is a hack/workaround, since that's how other editors have implemented "format region" with perltidy. And I suspect they do the same with other formatters too. It's also more efficient than sending the entire file, for example if we were editing a 10k+ lines file and we only want to format a function that's 10 lines.


Also, I thought this change would be useful for the package because of this line in the README:

atom-beautify sports a very impressive set of formatters. We should aspire to that level of coverage for Emacs.

Atom is now defunct, of course, but I took that statement to mean that we should strive to Emacs to achieve feature parity with editors such as VS Code and IntelliJ when it comes to formatters. In VS Code, you simply install the most popular Perl formatter extension and it supports formatting both the entire file or just a selection. And it does format selection by sending the text in the selection to perltidy and replacing it with the returned output, and it works very well. IntelliJ also supports formatting the selection; I haven't tested its Perl plugin but I'd be surprised if they don't implement "format selection" in the exact same way.

I wish the same would be possible in Emacs, which I why I wrote those changes. I've been using this "perltidy on region" from my branch for a month now with no problems. Format region is essential when you want to delegate the formatting of your code to a tool, but you're adding a few lines to someone else's code and you don't want to reformat the entire file along with it. So I think it would benefit many users if format-all supported this.

However if you disagree with this approach or you still think the drawbacks are not worth it feel free to close this.

lassik commented 1 year ago

https://github.com/perltidy/perltidy/issues/122

lassik commented 1 year ago

The perltidy-line-range-tidy branch adds pertidy --line-range-tidy support. Can you try whether it works for you?

tsilvap commented 1 year ago

Thank you for opening the issue to perltidy's upstream. I tested out the branch and it worked.

There's only one annoying thing, which is that when doing: C-a C-SPC C-3 C-n, the active region is three lines, but if we run M-x format-all-region, four lines will get reformatted instead of three (because it counts the line where point is, despite it not being a part of the active region).

But this is consistent with the behavior of comment-line (when run with an active region), so I think either way is fine.

lassik commented 1 year ago

Great.

I can see how that is annoying. comment-line does indeed comment the dangling last line, but comment-region does not. I rarely use region formating so I'm fine with either behavior, but it should be consistent across all formatters.

lassik commented 1 year ago

In any case, it seems like the perltidy-specific issue is solved. Please open another issue and/or PR if you'd like to change the last line behavior.