bram209 / leptosfmt

A formatter for the leptos view! macro
Apache License 2.0
274 stars 30 forks source link

Rust fmt and leptosfmt have different opinions #54

Closed johan-smits closed 1 year ago

johan-smits commented 1 year ago

When I have a component that is formatted with cargo-fmt:

// Valid Rust formatted code
#[component]
pub(crate) fn Error(cx: Scope, message: Option<String>) -> impl IntoView {
    view! { cx,
      <div>
        Example
      </div>
    }
}

When I format this with leptosfmt (0.1.11) It gives me a diff:

 pub(crate) fn Error(cx: Scope, message: Option<String>) -> impl IntoView {
     view! { cx,
-      <div>
-        Example
-      </div>
-    }
+    <div>
+      Example
+    </div>
+  }
 }

When I run the Rust format it changes it back again, since in our CI we test for cragofmt validation it fails if we use leptosfmt. It seems not to see the offset of the view starting position of the view and does not uses it to compensate it.

bram209 commented 1 year ago

Your example looks odd to me, so rustfmt indents the body of the macro with two spaces, while the rest of your code is formatted with 4 tab spaces?

Anyway, the following test passes:

    #[test]
    fn rustfmt_missmatch() {
        let source = indoc! {r#"
        // Valid Rust formatted code
        #[component]
        pub(crate) fn Error(cx: Scope, message: Option<String>) -> impl IntoView {
            view! { cx,
              <div>
                Example
              </div>
            }
        }
        "#};

        let result = format_file_source(source, Default::default()).unwrap();
        insta::assert_snapshot!(result, @r###"
        // Valid Rust formatted code
        #[component]
        pub(crate) fn Error(cx: Scope, message: Option<String>) -> impl IntoView {
            view! { cx,
                <div>
                    Example
                </div>
            }
        }
        "###);
    }
bram209 commented 1 year ago

if you are happy with the way leptosfmt formats things, you could disable body formatting for the view macro in rustfmt: https://rust-lang.github.io/rustfmt/?version=master&search=macro#skip_macro_invocations

johan-smits commented 1 year ago

@bram209 I have tab_spaces = 2 set in the toml. I thought this has only effect inside the view and not the view macro itself. When i set it to 4 it is correct.

johan-smits commented 1 year ago

You can reproduce it with this test:

   #[test]
    fn rustfmt_missmatch() {
        let source = indoc! {r#"
        // Valid Rust formatted code
        #[component]
        pub(crate) fn Error(cx: Scope, message: Option<String>) -> impl IntoView {
            view! { cx,
              <div>
                Example
              </div>
            }
        }
        "#};

        let result = format_file_source(source, FormatterSettings{tab_spaces: 2, ..Default::default()}).unwrap();
        insta::assert_snapshot!(result, @r###"
        // Valid Rust formatted code
        #[component]
        pub(crate) fn Error(cx: Scope, message: Option<String>) -> impl IntoView {
            view! { cx,
              <div>
                Example
              </div>
            }
        }
        "###);
    }
bram209 commented 1 year ago

@bram209 I have tab_spaces = 2 set in the toml. I thought this has only effect inside the view and not the view macro itself. When i set it to 4 it is correct.

leptosfmt is not aware of which settings you provide to rustfmt so therefore cannot know that you choose to indent your rust code with 4 spaces and your view macro with 2 spaces.

You need to do one of the following:

The current behaviour works as intended, so if you don't have further questions, I will close this issue?

johan-smits commented 1 year ago

If the answer to the next question is no you can close it.

Can leptosfmt see the number of spaces that where used for the initial macro start position and add that as a baseline? Like this you can position the initial macro anywhere you want and leptosfmt will format relative to this position. Like this it does not matter if you have different settings for rustfmt and leptosfmt.

Did you notice that the } of the view is also changed with leptosfmt and what I expected that it should do this right? By changing the position of the view } rustfmt also moves the body. See here the gif when I change the } and rustfmt after this.

Peek 2023-07-28 14-00 In the gif I'm simulating by hand what is causing byleptosfmt.

So if leptosfmt don't changes the } of the view or uses the same start position of the view macro for this closing bracket it works fine with 2 different settings.

bram209 commented 1 year ago

In your gif, are you running rustfmt or leptosfmt?

johan-smits commented 1 year ago

I run rustfmt. But the manual action I do removing the 2 spaces is what leptosfmt does (and also the body but that does not matter).

bram209 commented 1 year ago

can you try cargo install --git https://github.com/bram209/leptosfmt.git --branch auto-detect-rustfmt-indent

It will try to auto-detect the indent from the rust code. One thing I am not entirely happy about is that if the rust code is not formatted first, it might detect a weird indentation and will result in the view macro being improbably formatted. But this might not be an issue in practice.

Could you tell me if the above fix is working fine for you?

bram209 commented 1 year ago

Another option would be to simply pass a rust_tab_spaces as an argument to the cli (which defaults to tab_spaces, and add documentation in the readme that if you want to use a different indentation for the view macro than other rust code, you have to pass that argument.

One other question, would you expect that rust code that is within the view! macro to be indented with 4 spaces as well in this case?

johansmitsnl commented 1 year ago

The red is the auto-detect format, then green is when rust format runs:

     let title = if people.len() > 1 {
         view! {
-      <h2 class="text-3xl font-bold mb-12">
-        "Meet the " <span class="text-blue-600">"team"</span>
-      </h2>
-    }
+          <h2 class="text-3xl font-bold mb-12">
+            "Meet the " <span class="text-blue-600">"team"</span>
+          </h2>
+        }
     }

I don't know a lot of the internals, but can you see the position where view! starts on how many spaces this begins and use this as the offset?

bram209 commented 1 year ago

the position where view! starts on how many spaces this begins and use this as the offset

That is what I was doing before, but then you get issues like #20

I have to walk through the rust source code and somehow infer what the right indentation is, which is a pain in the ass.

awesomelemonade commented 1 year ago

the position where view! starts on how many spaces this begins and use this as the offset

That is what I was doing before, but then you get issues like #20

I have to walk through the rust source code and somehow infer what the right indentation is, which is a pain in the ass.

Not sure what you did before, but what you can try is locate the start byte (like here: https://github.com/bram209/leptosfmt/blob/a2b20ec66038ee5f515c7f36f83367cb8fdec471/formatter/src/source_file.rs#L49C13-L49C24) then find the byte index of the previous newline character, then copy the number of spaces that are directly after the newline character.

I do something similar in this project: https://github.com/awesomelemonade/expect-tests/blob/7ecf6a7a417ad44f93b52b2b99e2715ad5a2de62/src/lib.rs#L219 though looking back on it now, maybe I should've supported tabs? ehh rust uses spaces anyways

bram209 commented 1 year ago

the position where view! starts on how many spaces this begins and use this as the offset

That is what I was doing before, but then you get issues like #20 I have to walk through the rust source code and somehow infer what the right indentation is, which is a pain in the ass.

Not sure what you did before, but what you can try is locate the start byte (like here: https://github.com/bram209/leptosfmt/blob/a2b20ec66038ee5f515c7f36f83367cb8fdec471/formatter/src/source_file.rs#L49C13-L49C24) then find the byte index of the previous newline character, then copy the number of spaces that are directly after the newline character.

I do something similar in this project: https://github.com/awesomelemonade/expect-tests/blob/7ecf6a7a417ad44f93b52b2b99e2715ad5a2de62/src/lib.rs#L219 though looking back on it now, maybe I should've supported tabs? ehh rust uses spaces anyways

Hmm that could work, that would also fix the other issue. I will give this approach a try

bram209 commented 1 year ago

@johan-smits could you try to install: cargo install --git https://github.com/bram209/leptosfmt.git --branch fix-view-indentation-issues and let me know if that fixes your problem?

johansmitsnl commented 1 year ago

@bram209 this works :+1: