WhatsApp / erlfmt

An automated code formatter for Erlang
Apache License 2.0
412 stars 52 forks source link

Support for OTP 27 Sigils & Triple Quoted Strings #358

Closed maennchen closed 1 month ago

maennchen commented 2 months ago

Sigils: https://www.erlang.org/blog/highlights-otp-27/#sigils Triple Quoted Strings: https://www.erlang.org/blog/highlights-otp-27/#triple-quoted-strings

Also adds tests for documentation attributes. See: https://www.erlang.org/blog/highlights-otp-27/#overhauled-documentation-system

Resolves #355

eproxus commented 2 months ago

Lovely! I had just started on a similar PR myself 😄

Does it support triple quoted strings in documentation attributes? I didn't see a specific test case about that:

-module(foo).
-moduledoc """
The module.
""".

-doc """
The function.

Long description.
""".
f() -> ok.

Also, does it re-indent triple quoted strings. E.g. from:

f() ->
    """
the
  long
     string
""".

to

f() -> 
    """
    the
      long
         string
    """.

I used the following test module test/erlfmt_SUITE_data/triple_quoted_strings.erl:

-module(triple_quoted_strings).
-moduledoc """
abc
""".

-doc "foobar".
-type my_type :: integer().

-doc """
     baz
         extra
     """.
-opaque otype :: integer().

-doc "qux"
foo() ->
    """
    foo
    """.

bar() ->
"""
the
  long
     string
""".

Together with the desired result test/erlfmt_SUITE_data/triple_quoted_string.erl.formatted:

-module(triple_quoted_strings).
-moduledoc """
abc
""".

-doc "foobar".
-type my_type :: integer().

-doc """
     baz
         extra
     """.
-opaque otype :: integer().

-doc "qux"
foo() ->
    """
    foo
    """.

bar() ->
    """
    the 
      long
         string
    """.

as a test driver, though I'm not 100% sure about if the result will be correct since I never finished implementing the indentation logic.

maennchen commented 2 months ago

Does it support triple quoted strings in documentation attributes? I didn't see a specific test case about that:

Yes.

https://github.com/WhatsApp/erlfmt/pull/358/files#diff-ca49d93bca04b364c997af828c3aaf95beff334a9307b97fb1317b1721812ae4R4271

Also, does it re-indent triple quoted strings. E.g. from:

No, I haven't looked at indentation. But that would be a good feature to have.

I don't have the time right now to also get into this myself, but I would be very happy to collaborate to create a shared PR combined with your indentation logic. Do you want to send a PR to my PR branch? :D

eproxus commented 2 months ago

Does it support triple quoted strings in documentation attributes? I didn't see a specific test case about that:

Yes.

Ah, missed that. My bad!

#358 (files)

Also, does it re-indent triple quoted strings. E.g. from:

No, I haven't looked at indentation. But that would be a good feature to have.

I don't have the time right now to also get into this myself, but I would be very happy to collaborate to create a shared PR combined with your indentation logic. Do you want to send a PR to my PR branch? :D

Yeah, I can do that. I have the test and some "attempt" at getting indentation working. So far I added this hack but I have no clue if it is the right way to proceed or not:

diff --git a/src/erlfmt_algebra.erl b/src/erlfmt_algebra.erl
index 0f0994c..29ea433 100644
--- a/src/erlfmt_algebra.erl
+++ b/src/erlfmt_algebra.erl
@@ -522,6 +522,9 @@ format(Width, _, [{Indent, _, #doc_line{count = Count}} | T]) ->
     [NewLines, indent(Indent) | format(Width, Indent, T)];
 format(Width, Column, [{Indent, M, #doc_cons{left = X, right = Y}} | T]) ->
     format(Width, Column, [{Indent, M, X}, {Indent, M, Y} | T]);
+format(Width, Column, [{Indent, _, #doc_string{string = [$",$",$"|_] = S, length = L}} | T]) ->
+    [First|Rest] = string:split(S, <<"\n">>, all),
+    [First, [[indent(Column - string:length(First) - 1), R] || R <- Rest] | format(Width, Column, T)];
 format(Width, Column, [{_, _, #doc_string{string = S, length = L}} | T]) ->
     [S | format(Width, Column + L, T)];
 format(Width, Column, [{_, _, S} | T]) when is_binary(S) ->

Some feedback on that direction from you or the maintainers would be nice 😅

michalmuskala commented 2 months ago

Thank you for the PR, this is great!

I agree we do need to handle indentation of the tripple-quote strings before merging. The test example that @eproxus gave is a good one to include, however I see an inconsistency with:

-moduledoc """
abc
""".

-doc """
     baz
         extra
     """.

I believe both should end up indented like the -moduledoc to end up with:

-moduledoc """
abc
""".

-doc """
baz
    extra
""".

As to the particular approach, I'm generally fine with a hacky solution - the formatter has many of those already 😅

eproxus commented 2 months ago

@michalmuskala I added the second in the sense of that the tool should try to keep the existing desire of the user if possible (as is done with line breaks somewhat). But perhaps for indentation that rule is not really applied now that I think about it...

michalmuskala commented 2 months ago

@eproxus in general we retain line breaks introduced by the user, but we do re-indent everything according to the rules

eproxus commented 2 months ago

@michalmuskala Makes sense!

@maennchen Do you want a PR with my stuff right now to your branch, or would you rather fix the comments first?

maennchen commented 2 months ago

@eproxus Fixes are done. Go ahead :)

michalmuskala commented 1 month ago

Is this ready to merge? Or should I wait for the work on indentation?

eproxus commented 1 month ago

I think the PR is good enough to add compatibility for OTP 27. I got stuck in the rabbit whole of actually implementing indentation and didn't succeed yet. @maennchen Do you want my half-working code there?

maennchen commented 1 month ago

@michalmuskala We can merge this without indentation support. 👍

@eproxus I don't plan on working on the indentation. But if anyone else wants to, it might be good to push the code somewhere so that they can look at the work you already did.

michalmuskala commented 1 month ago

I'll look into properly handling indentation myself

Thank you both for working on this! ❤️

michalmuskala commented 1 month ago

@maennchen I noticed you implemented conversion of single-line tripple-quoted strings into a regular string. I'm not sure we should actually do that - in general we try to preserve the "style" of an expression the user wrote. I'm leaning towards removing this conversion

michalmuskala commented 1 month ago

Support for indentation was implemented in https://github.com/WhatsApp/erlfmt/pull/362 and I cut a 1.5.0 release with support for the new OTP 27 features