bram209 / leptosfmt

A formatter for the leptos view! macro
Apache License 2.0
253 stars 27 forks source link

Indentation issue #140

Closed dominikwilkowski closed 1 week ago

dominikwilkowski commented 2 weeks ago

Hi there,

If this is intended behavior please feel free to close this. Not trying to open up a discussion about indentation here but this SEEMS to be a bug to me?

I got this leptos component below and it seems that all indentation after the on:click closure is doubled up.

#[component]
pub fn Foo() -> impl IntoView {
    view! {
        <button on:click=move |_| {
                wasm_bindgen_futures::spawn_local(async {
                        let video_element = web_sys::window()
                                .unwrap()
                                .document()
                                .unwrap()
                                .get_element_by_id("foobar")
                                .unwrap()
                                .dyn_into::<HtmlVideoElement>()
                                .unwrap();
                        do_the_thing(video_element);
                });
        }>Click Me</button>
    }
}

Here a screenshot with whitespace enabled:

image

leptosfmt with settings:

λ leptosfmt src/**/*.rs
max_width = 80
tab_spaces = 2
indentation_style = "Tabs"
newline_style = "Unix"
attr_value_brace_style = "WhenRequired"
closing_tag_style = "Preserve"
macro_names = [
    "leptos::view",
    "view",
]

[attr_values]

✅ src/foo/bar.rs
[...]
ℹ️ Formatted 10 files in 2 ms

On macos latest ARM

bram209 commented 2 weeks ago

Keep in mind that leptosfmt only formats the view! macro. We do not touch indentation of other rust source code.

So this is intended behavior, if you set indentation_style to

You set the indentation style to two tabs, which is exactly what you see in your screenshot. Maybe you meant to use an indentation of two spaces instead?

So in summary:

If you don't want to have two different indentation style between the two formatters (rustfmt & leptosfmt), use leptosfmt's Auto setting. You can configure rustfmt to format with your preferred style and leptosfmt will automatically detect it.

dominikwilkowski commented 2 weeks ago

Oh sorry I don't think it's super easy to see so I made this to illustrate it again a bit better:

indentation

I am using tabs everywhere for indentation. But via leptosfmt after <button on:click=move |_| { it uses two tabs instead of one for each indentation. Does that make sense?

The way it SHOULD format this is:

#[component]
pub fn Foo() -> impl IntoView {
    view! {
        <button on:click=move |_| {
            wasm_bindgen_futures::spawn_local(async {
                let video_element = web_sys::window()
                    .unwrap()
                    .document()
                    .unwrap()
                    .get_element_by_id("foobar")
                    .unwrap()
                    .dyn_into::<HtmlVideoElement>()
                    .unwrap();
                do_the_thing(video_element);
            });
        }>Click Me</button>
    }
}
Screenshot 2024-08-30 at 6 17 05 AM
dominikwilkowski commented 2 weeks ago

Just tried spaces and it seems to have the same issue?

#[component]
pub fn QrScanner() -> impl IntoView {
    view! {
    <script src="js/qr-scanner.umd.min.js"></script>
    <script src="js/qr_scanner_wrapper.js"></script>
    <div id="qr-video-container">
      <video id="qr-video"></video>
    </div>
    <button on:click=move |_| {
        wasm_bindgen_futures::spawn_local(async {
            let video_element = web_sys::window()
                .unwrap()
                .document()
                .unwrap()
                .get_element_by_id("qr-video")
                .unwrap()
                .dyn_into::<HtmlVideoElement>()
                .unwrap();
            start_qr_scanner(video_element);
        });
    }>Start QR Scanner</button>
  }
}
Screenshot 2024-08-30 at 6 43 32 AM
bram209 commented 2 weeks ago

There was an issue where the formatter settings were not used when formatting rust code embedded in the view! macro. I created a fix, could you try it out and let me know if this resolves your issues?

Install this version with: cargo install --git https://github.com/bram209/leptosfmt.git --branch fix/indentation-of-syn-exprs

dominikwilkowski commented 2 weeks ago

I installed that and now we have another bug :) For the affected indentations it now uses spaces instead of tabs but only deeper inside?

#[component]
pub fn QrScanner() -> impl IntoView {
    view! {
    <script src="js/qr-scanner.umd.min.js"></script>
    <script src="js/qr_scanner_wrapper.js"></script>
    <div id="qr-video-container">
      <video id="qr-video"></video>
    </div>
    <button on:click=move |_| {
      wasm_bindgen_futures::spawn_local(async {
        let video_element = web_sys::window()
          .unwrap()
          .document()
          .unwrap()
          .get_element_by_id("qr-video")
          .unwrap()
          .dyn_into::<HtmlVideoElement>()
          .unwrap();
        start_qr_scanner(video_element);
      });
    }>Start QR Scanner</button>
  }
}
Screenshot 2024-08-31 at 7 52 58 AM

For the avoidance of doubt it ran with these settings:

λ leptosfmt src/**/*.rs
max_width = 80
tab_spaces = 2
indentation_style = "Tabs"
newline_style = "Unix"
attr_value_brace_style = "WhenRequired"
closing_tag_style = "Preserve"
macro_names = [
    "leptos::view",
    "view",
]

[attr_values]

✅ src/app/app.rs
[...]
bram209 commented 2 weeks ago

Each tab is 4 spaces. tab_spaces is the indentation size in spaces. If the indentation style is set to "Tabs", the formatter will print pending indentation / tab_spaces hard tabs (\t) and the remainder (pending indentation % tab_spaces) in spaces.

In your case, you set it to two spaces AND the style to "Tabs", so that is why it prints two spaces (the remainder) instead of tabs. If you want 1 hard tab as indentation size, you should set tab_spaces to 4 and indentation_style to "Tabs", if you want two hard tabs, set tab_spaces to 8, etc.

When style is "Tabs" and tab_spaces is 4, I get the following output:

image
bram209 commented 2 weeks ago

Each tab is 4 spaces. tab_spaces is the indentation size in spaces. If the indentation style is set to "Tabs", the formatter will print pending indentation / tab_spaces hard tabs (\t) and the remainder (pending indentation % tab_spaces) in spaces.

In your case, you set it to two spaces AND the style to "Tabs", so that is why it prints two spaces (the remainder) instead of tabs. If you want 1 hard tab as indentation size, you should set tab_spaces to 4 and indentation_style to "Tabs", if you want two hard tabs, set tab_spaces to 8, etc.

And just to be clear, this behavior changed in the PR: https://github.com/bram209/leptosfmt/pull/141/files#diff-0f5d6c9158d2f81cabfc410d365290fe8765b525df73cee8565a0128cd4786dfR365-R366

dominikwilkowski commented 2 weeks ago

Interesting. That is different to how cargo fmt does it but that's ok. Thank you. At least I know now how to fix it :)

dominikwilkowski commented 1 week ago

Thinking about this again, I could be entirely wrong here but I always assumed that tab_spaces is there to be able to calculate the max width of a line. Since tabs can be represented as any number of spaces it needs this setting to be able to calculate the length of a line. It should reflect what your editor shows your tabs as.

bram209 commented 1 week ago

So currently, max_width represents the visual max width of a line, where we assume that each hard tab is displayed as the size of 4 spaces.

You have a point that editors can be configured to show a hard tab in different sizes, are you saying that you expect the max_width configuration to count each hard tab (indentation_style = "Tabs") as tab_spaces characters, but the indentation is always a single tab?

The only problem I would have with this is that if you work with multiple people on a project, one person can have a display size of 6 spaces, the other can have it set to 2 (as it's purely a visual setting). What would be the value of tab_spaces? If both people provided their own configuration, the formatter would then be formatting with different max line widths, which would be the last thing that you want?

That is different to how cargo fmt does it but that's ok.

If rustfmt does this differently, I would be welcoming a PR that follows rustfmt behavior more closely to avoid confusion for users of leptosfmt.

dominikwilkowski commented 1 week ago

are you saying that you expect the max_width configuration to count each hard tab (indentation_style = "Tabs") as tab_spaces characters, but the indentation is always a single tab

The only problem I would have with this is that if you work with multiple people on a project, one person can have a display size of 6 spaces, the other can have it set to 2 (as it's purely a visual setting). What would be the value of tab_spaces? If both people provided their own configuration, the formatter would then be formatting with different max line widths, which would be the last thing that you want?

The problem you bring up is exactly why you need another config to say what the size of a tab is in order for a max_width config to be deterministic across multiple editors. No you can't make it work for everyone and will have to set a project level setting to say what the size of a tab is. It's like now where you default to 4. Instead of the hard-coded 4 you take tab_spaces (and if tab_spaces isn't set it should default to 4 anyway).

It doesn't solve that the very nature of tabs is ambiguous and is intended to be rendered as whatever you like on different editors because that would break max_width but it does allow you to set a project default to say "hey let's all agree that a tab will be 2 spaces wide so that we can all have consistent max_width".

If rustfmt does this differently

I was actually not 100% sure so I just dug in and it turns out their code is very hard to read 🙃 so I settled for an experiment: Set hard_tabs to true and set the max_width to something small (so it can have enough impact) and then format via cargo fmt -- -l. Now change ONLY the tab_spaces and run it again. The result is: tab_spaces has indeed an impact on how the max_width is calculated and will change the output of the formatter. So while I didn't look into the code deep enough this confims to me that the size of a tab can't be hard-coded and would take tab_spaces into account.

If you're happy with all that, I'm happy to look into making a PR for leptosfmt

bram209 commented 1 week ago

That sounds reasonable. I don't think we need to have an additional configuration option, if we accept the constraint that when using indentation_style = "Tabs", a single indentation level will always be a single hard-tab \t (but you can still influence the max_width of a line through the tab_spaces option).

As for the PR, I think this relatively small change should do it:

143

Would you mind double-checking if this matches your expectations? cargo install --git https://github.com/bram209/leptosfmt.git --branch fix-hard-tab-size

dominikwilkowski commented 1 week ago

Nice. The PR looks good. I tested it locally and it works as I would expect too. Thanks