Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update formatters with changes from OTP 26 #364

Merged
merged 13 commits into from
Oct 28, 2024

Conversation

elbrujohalcon
Copy link
Collaborator

@elbrujohalcon elbrujohalcon commented Oct 22, 2024

The original intention of this PR was upgrading only the otp_formatter to work with OTP 26 and 27 so a future PR could update the other formatters. But I quickly realized that, in order to run tests with newer versions of OTP, I needed to update all the formatters.
So… missing tasks:

  • Upgrade dependencies
  • Update other formatters
  • Get tests to pass on both OTP 25 and OTP 26
  • Remove OTP 24 from github's workflows and set min-version to 25
  • Add tests for OTP26 syntax:
    • map comprehensions
    • Compare OTP's lib/syntax_tools/test/syntax_tools_SUITE_data/ between versions.

@elbrujohalcon elbrujohalcon changed the title Update otp_formatter with changes from OTP 26 and 27 Update formatters with changes from OTP 26 and 27 Oct 24, 2024
@elbrujohalcon elbrujohalcon force-pushed the elbrujohalcon.otp_formatter.update branch from dfe05fb to 80dd875 Compare October 24, 2024 12:44
inline_clause_bodies => true,
paper => 50,
sub_indent => 8}.
-format #{break_indent => 1}.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and other similar changes are due to a subtle bug (fixed in OTP26's erl_syntax) that reordered map items when they were found in attributes. I decided to just put the items in the order in which erl_syntax returns them (thus, hiding the bug in OTP25) so that tests run seamlessly in 25 and 26.

@elbrujohalcon elbrujohalcon changed the title Update formatters with changes from OTP 26 and 27 Update formatters with changes from OTP 26 Oct 25, 2024
@elbrujohalcon elbrujohalcon marked this pull request as ready for review October 25, 2024 13:57
@elbrujohalcon
Copy link
Collaborator Author

elbrujohalcon commented Oct 25, 2024

My original goal was to move us all the way to OTP27…
But… OTP tools are terrible for parsing OTP27-compatible code.

Look a this module…

-module(otp27).
-moduledoc """
    New stuff introduced in OTP27.
""".

-export([single_line/0, attributes/0, external/0, markdown/0, false/0, sigils/0]).

-doc "Documentation in one line".
single_line() -> ok.

-doc "You can use".
-doc #{attributes => #{as => maps}}.
attributes() -> {as, maps}.

-doc {file, "external.md"}.
external() -> {in, a, file}.

-doc """
Can contain markdown...
# Title
## Subtitle
### Level 3
**bold**, _italic_
""".
markdown() -> ok.

-doc false.
false() -> false.

sigils() ->
    {
        ~"This is valid now",
        ~B[<<"This, too">>],
        ~b"This one, as well",
        ~"""
        This is a multi-line
                string that can have a quotation mark: "
        as the one in the previous row
        """,
        ~s|Confusingly enough, this is also a string|,
        'not a string,'
    }.

This is how epp_dodger + erl_prettypr would format it:

-module(otp27).

-moduledoc("    New stuff introduced in OTP27.").

-export([single_line/0,
         attributes/0,
         external/0,
         markdown/0,
         false/0,
         sigils/0]).

-doc("Documentation in one line").

single_line() -> ok.

-doc("You can use").

-doc(#{attributes => #{as => maps}}).

attributes() -> {as, maps}.

-doc({file, "external.md"}).

external() -> {in, a, file}.

-doc("Can contain markdown...\n# Title\n## "
     "Subtitle\n### Level 3\n**bold**, _italic_").

markdown() -> ok.

-doc(false).

false() -> false.

sigils() ->
    {<<"This is valid now"/utf8>>,
     <<"<<\"This, too\">>"/utf8>>,
     <<"This one, as well"/utf8>>,
     <<"This is a multi-line\n        string "
       "that can have a quotation mark: \"\nas "
       "the one in the previous row"/utf8>>,
     "Confusingly enough, this is also a string",
     'not a string,'}.

And this is how ktn_dodger + a slightly adjusted default_formatter would format it like this…

-module(otp27).

-moduledoc "    New stuff introduced in OTP27.".
-export([single_line/0, attributes/0, external/0, markdown/0, false/0, sigils/0]).

-doc "Documentation in one line".
single_line() ->
    ok.

-doc "You can use".
-doc #{attributes => #{as => maps}}.
attributes() ->
    {as, maps}.

-doc {file, "external.md"}.
external() ->
    {in, a, file}.

-doc "Can contain markdown...\n# Title\n## Subtitle\n### Level 3\n**bold**, _italic_".
markdown() ->
    ok.

-doc false.
false() ->
    false.

sigils() ->
    {<<"This is valid now"/utf8>>,
     <<"<<\"This, too\">>"/utf8>>,
     <<"This one, as well"/utf8>>,
     <<"""
        This is a multi-line
                string that can have a quotation mark: "
        as the one in the previous row
        """/utf8>>,
     "Confusingly enough, this is also a string",
     'not a string,'}.

That's because the parsers do not return information about…

  • How sigils were written in the first place. They are all returned as binaries with utf8 strings in them.
  • How heredoc strings were written in the first place. You can't distinguish between someone writing \n or using multiple lines to write the string.

@elbrujohalcon elbrujohalcon merged commit ebe6210 into main Oct 28, 2024
5 checks passed
@elbrujohalcon elbrujohalcon deleted the elbrujohalcon.otp_formatter.update branch October 28, 2024 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants