bram209 / leptosfmt

A formatter for the leptos view! macro
Apache License 2.0
271 stars 29 forks source link

[Feature Request] Format ident attr values without braces #139

Open BrandonDyer64 opened 2 months ago

BrandonDyer64 commented 2 months ago

It makes sense to have an option that doesn't put braces around numbers and strings, but It's a bit weird to see it then put braces around simple variable names.

Current:

<Component
    closure={move || 5} // Braces
    math={5 + 5}        // Braces
    number=5            // No Braces
    variable={some_var} // Braces?
/>

Desired:

<Component
    closure={move || 5} // Braces
    math={5 + 5}        // Braces
    number=5            // No Braces
    variable=some_var   // No Braces
/>
bram209 commented 2 months ago

When I have a leptosfmt.toml with:

attr_value_brace_style = "AlwaysUnlessLit"

And run:

echo "fn test() { view! { <Component closure={move || 5} math={5 + 5} number={5} variable={some_var} /> } }" | leptosfmt --stdin --rustfmt --max-width=60

The output is:

fn test() {
    view! {
        <Component
            closure={move || 5}
            math={5 + 5}
            number=5
            variable={some_var}
        />
    }
}

Are you sure that you are configuring leptosfmt correctly? I am closing this issue, but feel free to reopen if you can't get it to work.

BrandonDyer64 commented 2 months ago

@bram209 Yes I am. In fact, your output shows the problem.

fn test() {
    view! {
        <Component
            closure={move || 5}
            math={5 + 5}
            number=5
            variable={some_var} // <- This has curly braces, but there should be a config option that makes this not happen
        />
    }
}
BrandonDyer64 commented 2 months ago

@bram209 I'm not able to reopen this

bram209 commented 2 months ago

Ah my bad, when I glanced over this issue I didn't see that you actually want to not have curly braces when you pass a plain identifier. However, AlwaysUnlessLit will always put braces unless it's a literal expression, a variable identifier counts as a regular expression.

I could be open to adding a Custom variant, where you could specify with more granularity which expressions are to be formatted with braces and which without, like so:

attr_value_brace_style = "Custom"
attr_value_braces_exclude = [ "Array", "Range", "Literal",  "Ident" ] // with other options like "If", "Binary" (your `5 + 5` example), "Path"...

But one thing that I would like to avoid, is having leptos code being formatted in many different ways. I would love if leptos, as a community, could come to terms with a sane default to format things. What are your thoughts on this?

Edit: there was also discussion to enforce braces altogether (better RA support), so I reached out on discord to get some more opinions on this feature request: https://discord.com/channels/1031524867910148188/1031524868883218474/1279359661606436874

BrandonDyer64 commented 2 months ago

But one thing that I would like to avoid, is having leptos code being formatted in many different ways. I would love if leptos, as a community, could come to terms with a sane default to format things.

I absolutely agree with this sentiment, which is why I had originally requested it be added to AlwaysUnlessLit. In my opinion, if there is going to be an option that mixes braces and non-braces then it is sane to include idents with that.

I personally prefer it without braces in general, but still want to keep braces for expressions and closures.

bram209 commented 2 months ago

I personally prefer it without braces in general, but still want to keep braces for expressions and closures.

I personally prefer it with braces in general, but still want to omit braces for literal values. So my mixed feelings are:

What if I deprecate attr_value_brace_style in favor for:

default_attr_value_brace_style = "WhenRequired" # "WhenRequired", "Preserve", "Always"

# One or multiple `attr_value_brace_style` blocks, where you override styles for specific kind of expressions
[[attr_value_brace_style]] 
style = "Always" 
for = ["Block", "Closure"]

Would that work for you?

BrandonDyer64 commented 2 months ago

a variable identifier is not a literal value, unlike the name "AlwaysUnlessLit" suggests

This is why I was more suggesting to have another option. My gripe was specifically that there is an option that doesn't put braces around literals but does put them around idents, especially when there is no option otherwise.

For consistency's sake, I think idents and literals should just be treated the same.

I just think it makes sense to have an option (regardless of what it's called) that end up with the following:

<Component
    closure={move || 5} // Braces
    math={5 + 5}        // Braces
    string="hello"      // No Braces
    number=5.0          // No Braces
    variable=some_var   // No Braces
/>

I really expected the controversial part of this issue not to be whether this is an option that should exist, but what should happen with dots or simple function calls. What I'm really looking for is braces when the separation between one attribute and another isn't clear. So, more of a controversial take potentially, I think braces should be omitted from things with dots and calls, but included for anything that would be formatted with spaces (math expressions and closure defs). Example:

<Component
    dot=some_var.x          // No Braces
    call=fun()              // No Braces
    dot_call=some_var.fun() // No Braces
    variable=some_var       // No Braces
    closure={move || 5}     // Braces
    math={5 + 5}            // Braces
/>

I personally prefer it with braces in general, but still want to omit braces for literal values.

I used to have the same preference, but given the Leptos documentation is the way it is, people on my team started writing it without braces and my preferences were forced to change. Now I tend to write without braces.