Try to converge in one pass when formatting a style attr with malva

[UnknownPlatypus]

Currently the following snippet need two pass to format:

<p
  style="
    margin: 0;
    font-size: 14px;
    display: flex;
    flex-direction: column-reverse;
  "
>
    aaaa
</p>

# First pass
<p 
  style="margin: 0; font-size: 14px; display: flex; flex-direction: column-reverse"
>
  aaaa
</p>

# Second pass
<p style="margin: 0; font-size: 14px; display: flex; flex-direction: column-reverse">
  aaaa
</p>

This happens because we choose to add a newline or not based on the initial attr content. At first it contains a newline but the formatting removes it, hence the second pass.

The current fix is not perfect because it will slightly change the formatting if you don't use an external formatter. Do you have an idea on how to know if the formatted text will end on multiple lines or not from is_multi_line_attr ?

[g-plane]

You're right, and I'm thinking if there's a better solution.

[g-plane]

We can move the "style" attribute detection here: https://github.com/g-plane/markup_fmt/blob/34be738967bed232cb3c4cf6a8dc9ab653229949/markup_fmt/src/printer.rs#L477

-                    && !is_multi_line_attr(attr) =>
+                    && !is_multi_line_attr(attr)
+                    && !matches!(attr, Attribute::Native(attr) if attr.name == "style") =>

[UnknownPlatypus]

We can move the "style" attribute detection here:

I'm not sure I follow. If I try your exemple, then first pass never goes to second pass.

However, I can do that to extract the check outside and retain this PR behavior but I'm not sure it matters:

if !is_whitespace_sensitive
    && (!is_multi_line_attr(attr)
    // External formatter (Malva) always format style attr on a single line (see `singleLineTopLevelDeclarations`)
    || matches!(attr, Attribute::Native(attr) if attr.name.eq_ignore_ascii_case("style"))) =>

To have the correct behavior every time, we would need to know from evaluating attr.doc(ctx, &state) if it spans multiple lines but I'm not even sure we can do that ?

[g-plane]

I've come up with several solutions, but none of them are perfect:

1. When processing attributes, modify the state to record if it contains line breaks. This needs &State to be &mut State which affect so much code, or use Cell to cheat ourselves. Also, there're several kinds of attributes in Attribute enum. Adding these logic to all branches of code will make it hard to maintain in the future.
2. After calling attr.doc(..), recursivly check if there're Doc::NewLine or Doc::EmptyLine. However, though Doc::NewLine and Doc::EmptyLine are accessible, they're not designed for public use. In other words, changes to these enum variants of Doc won't be considered as breaking changes, and there're no guarantees about the usage of these enum variants. (Imagine they're removed in the future.)
3. Change Doc::space() to Doc::line_or_space() before that single attribute and remove the is_multi_line_attr detection. This can make sure that if there're line breaks in that attribute we will insert a line break, but once the attribute is too long, line break will be inserted too. This will make single_attr_same_line option become unuseful.
4. Remove this special case for single attribute. This will deprecate single_attr_same_line option.

[UnknownPlatypus]

I don't think 4 is great, I.de rather have this not converge in one pass in rare cases than lose the feature.

Option 2 is maybe nice, even the API can change, it most likely won't and if it does, it would only require minor changes for this feature ?

There is also option 5 which is the current state of the pr. It means assume the external formatter will format style attribute on one line

[g-plane]

Option 2 is maybe nice, even the API can change, it most likely won't and if it does, it would only require minor changes for this feature ?

No, they aren't APIs, so I said they're not for public use. Changes to them aren't breaking changes.

Actually this PR is not perfect, too. First, it assumes external formatter is existed. There may be some users who don't use Malva. (Yes, I believe most users won't use markup_fmt without Malva.) Second, this only solves for style attribute, but other attributes have same problem.

[UnknownPlatypus]

No, they aren't APIs, so I said they're not for public use. Changes to them aren't breaking changes.

Sorry, I said that poorly. What I wanted to say is that we might be ok with relying on these privates because it's would be very scoped in markup_fmt

Actually this PR is not perfect, too. First, it assumes external formatter is existed.

Yeah, and also assumes malva will not change it's setting about multi-lines. We could probably check if an external formatter is available and decide based on that but I'm not sure we can access the external formatter config ?

Second, this only solves for style attribute, but other attributes have same problem.

Yes i do agree this is a more general issue that we will face for other attributes requiring external formater. pretty_jinja integration might make this issue come up more often.

But still I feel like option 3/4 will bring back the issue outlined in https://github.com/g-plane/markup_fmt/pull/42

[g-plane]

we might be ok with relying on these privates

I wouldn't consider using private items. This is not an effective solution.

We could probably check if an external formatter

Sadly we can't. dprint doesn't provide such capabilities.