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

Feature request: --lines to support partial formatting #114

Closed
maleadt opened this issue Nov 29, 2024 · 7 comments · Fixed by #120
Closed

Feature request: --lines to support partial formatting #114

maleadt opened this issue Nov 29, 2024 · 7 comments · Fixed by #120

Comments

@maleadt
Copy link
Contributor

maleadt commented Nov 29, 2024

clang-format has the following feature:

  --lines=<string>               - <start line>:<end line> - format a range of
                                   lines (both 1-based).
                                   Multiple ranges can be formatted by specifying
                                   several -lines arguments.
                                   Can't be used with -offset and -length.
                                   Can only be used with one input file.

This empowers git-clang-format, a script that's used to incrementally format a codebase by only formatting the changes one is about to commit. Would it be possible to add that to Runic too?

@fredrikekre
Copy link
Owner

This would definitely be good to have but not trivial to implement because Runic is currently completely oblivious of line numbers. Do you know what happens if the range don't cover full expressions, for example half a function?

only formatting the changes one is about to commit

As opposed to existing pre-commit functionality which operate on the whole modified file?

In neovim I use conform.nvim which, even if the formatters don't support range formatting, enables this by inspecting the diff, see https://github.com/stevearc/conform.nvim/blob/master/doc/advanced_topics.md#range-formatting. In my experience this works pretty well. This would probably be the simplest addition.

@maleadt
Copy link
Contributor Author

maleadt commented Nov 29, 2024

As opposed to existing pre-commit functionality which operate on the whole modified file?

Correct.

I was following the design of git-clang-format, which inspects the current diff, parses out the chunk range markers, and uses those to invoke clang-format with. That seems like a more accurate design, as opposed to the "aftermarket" range formatting (words from the conform.nvim docs, not mine).

Do you know what happens if the range don't cover full expressions, for example half a function?

It only formats half the function then. The logic for that is in https://github.com/llvm/llvm-project/blob/main/clang/lib/Format/AffectedRangeManager.h and https://github.com/llvm/llvm-project/blob/main/clang/lib/Format/AffectedRangeManager.cpp

@fredrikekre
Copy link
Owner

Thats probably a good way to find what to pass to --lines, but it is still a bit tricky to determine what tree nodes fall in the range, for example

$ cat main.c
int main(void){
    int x = 1;
    return 0;
 }
 
$ clang-format --lines=1:1 -i main.c

$ git diff
diff --git a/main.c b/main.c
index 135e7ac..101ec75 100644
--- a/main.c
+++ b/main.c
@@ -1,4 +1,4 @@
-int main(void){
+int main(void) {
     int x = 1;
     return 0;
- }
+}

Perhaps the simplest thing is to only format tree nodes which fall entirely inside the range. This would be similar to adding # runic: on/off on the line ranges. (I believe this is how the LanguageServer.jl does it julia-vscode/LanguageServer.jl#1190)

@maleadt
Copy link
Contributor Author

maleadt commented Dec 2, 2024

Perhaps the simplest thing is to only format tree nodes which fall entirely inside the range.

That seems reasonable, yes. It's not like clang-format is entirely consistent either:

❯ cat test.c
int main(void){
    foo ;
    bar ;
    baz ;
 }

❯ clang-format --lines=2:2 test.c
int main(void){
  foo;
  bar;
  baz;
 }

❯ clang-format --lines=3:3 test.c
int main(void){
    foo ;
    bar;
    baz ;
 }

On the other hand, when changing e.g. an argument to a function call spanning multiple lines, it would be nice if the entire call was formatted. But maybe that's focusing too much on details here, and it'd be good to start with a simple solution first.

@fredrikekre
Copy link
Owner

One option would be to walk up the syntax tree to toplevel, but that would mean that the entire function would be formatted (but maybe that is fine).

@maleadt
Copy link
Contributor Author

maleadt commented Dec 3, 2024

That's probably a little too drastic? It could mean that most of the source file gets formatted, e.g., when using a coarse, toplevel if VERSION check. Maybe a more nuanced approach would be to walk the tree upwards, but abort as soon as it encounters an Expr(:block) (or however that's represented by JuliaSyntax), which should cover constructs that may contain expressions falling far outside the intended range.

@fredrikekre
Copy link
Owner

Yea. I think the language server approach of inserting comments would work well because I don't ever think Runic transposes comments with some other tokens.

fredrikekre added a commit that referenced this issue Dec 6, 2024
This patch implements the `--lines=a:b` command line argument for
limiting the formatting to the line range `a:b`. Multiple ranges are
supported. Close #114.
fredrikekre added a commit that referenced this issue Dec 7, 2024
This patch implements the `--lines=a:b` command line argument for
limiting the formatting to the line range `a:b`. Multiple ranges are
supported. Close #114.
fredrikekre added a commit that referenced this issue Dec 9, 2024
This patch implements the `--lines=a:b` command line argument for
limiting the formatting to the line range `a:b`. Multiple ranges are
supported. Closes #114.
fredrikekre added a commit that referenced this issue Dec 9, 2024
This patch implements the `--lines=a:b` command line argument for
limiting the formatting to the line range `a:b`. Multiple ranges are
supported. Closes #114.
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 a pull request may close this issue.

2 participants