Skip to content

format sigils using applicable plugins#37

Closed
superhawk610 wants to merge 2 commits intoremoteoss:mainfrom
superhawk610:fix/format-sigils
Closed

format sigils using applicable plugins#37
superhawk610 wants to merge 2 commits intoremoteoss:mainfrom
superhawk610:fix/format-sigils

Conversation

@superhawk610
Copy link
Copy Markdown
Contributor

Closes #36.

This PR modifies the .ex / .exs format process to include plugins that are capable of formatting sigil contents, e.g. Phoenix.LiveView.HTMLFormatter for the ~H sigil.

See https://github.com/elixir-lang/elixir/blob/d67cb9634baba80e61188da5a688c9b127f94038/lib/mix/lib/mix/tasks/format.ex#L337 for corresponding implementation in mix format.

Copy link
Copy Markdown
Member

@JesseHerrick JesseHerrick left a comment

Choose a reason for hiding this comment

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

Hi @superhawk610, thank you very much for your contribution! I just have one note about unifying things here.

Comment thread internal/lsp/formatter_server.exs Outdated
Process.group_leader(self(), old_gl)
end
else
sigils =
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that this logic could actually be built above where we accumulate applicable plugins. Unless I'm misunderstanding it, we just need to add the additional sigil config to the plugin.format calls. As-is, this code is doing very similar logic to the branch directly above. I think this can be unified. This is all the more important because our protocol breaks if we leak any data to IO during the failed formatting, hence the logic in the try block.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The sigils option is passed to Code.format_string! (took me a bit to figure that out when reading through the mix format source, since it's undocumented). That means both arms of the if statement will now potentially call plugin.format, so I moved the entire statement into the try block to make sure nothing leaks to stdout. I also moved the sigil collection into the same loop that checks extensions.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hey @superhawk610, why do we need to call basically the same logic in the if and else branches? Couldn't we just combine and dynamically build the options? Perhaps I'm missing something.

Copy link
Copy Markdown
Contributor Author

@superhawk610 superhawk610 Apr 22, 2026

Choose a reason for hiding this comment

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

In the if branch (when we're formatting non-Elixir files), the Enum.reduce expands to something like

file_contents
|> plugin_a.format(opts)
|> plugin_b.format(opts)
|> plugin_c.format(opts)

Note that we're calling plugin.format() with the file contents directly, and not calling Code.format_string!().

In the else branch (when we're formatting Elixir source files), the call expands to something like

Code.format_string!(file_contents, sigils: [
  # format ~H"" contents
  H: fn input, sigil_H_opts ->
    input
    |> plugin_H_a.format(opts ++ sigil_H_opts)
    |> plugin_H_b.format(opts ++ sigil_H_opts)
  end,
  # format ~S"" contents
  S: fn input, sigil_S_opts ->
    input
    |> plugin_S_a.format(opts ++ sigil_S_opts)
    |> plugin_S_b.format(opts ++ sigil_S_opts)
  end
])

Note that we are calling Code.format_string!() and plugins are configured within the sigils option, grouped by sigil type, and passed in only the sigil's contents (not the full file contents) and additional options that change per sigil. The anonymous function will be invoked once for each instance of that sigil within the source file.

The contents of the Enum.reduce look similar, but are doing (slightly) different things.

Here's the source for mix format where they're also split into two separate Enum.reduce calls:

Non-Elixir files:

https://github.com/elixir-lang/elixir/blob/d67cb9634baba80e61188da5a688c9b127f94038/lib/mix/lib/mix/tasks/format.ex#L684

Elixir files with sigils:

https://github.com/elixir-lang/elixir/blob/d67cb9634baba80e61188da5a688c9b127f94038/lib/mix/lib/mix/tasks/format.ex#L349

If you'd like to refactor to consolidate them, please feel free! I tried to stay as close to the mix format implementation as I could.

This should avoid polluting stdout with any exception messages.
@superhawk610
Copy link
Copy Markdown
Contributor Author

Closing in favor of #55 - thanks @JesseHerrick!

JesseHerrick added a commit that referenced this pull request May 1, 2026
Continuation of #37 #36. I can't push to that branch, so adding the
needed changes here.

- Add sigil formatter support to the persistent BEAM formatter by wiring
plugin-advertised sigils into `Code.format_string!/2`.
- Keep whole-file plugin formatting scoped to plugins that explicitly
match the file extension, while `.ex` / `.exs` files use Elixir
formatting with sigil callbacks.
- Add HEEX-focused regression coverage for a
`Phoenix.LiveView.HTMLFormatter`-compatible plugin formatting both
`.heex` files and `~H` sigils inside Elixir files.

Co-authored-by: Aaron Ross <superhawk610@gmail.com>
@JesseHerrick
Copy link
Copy Markdown
Member

I just merged #55 and added you as a co-author since you kicked off these changes @superhawk610. Thanks again for the contribution!

These changes will be included in the next release. First, I plan on merging a few more improvement MRs before creating the next release. In the meantime, feel free to use main building from source.

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.

Formatter doesn't handle sigils like ~H in .ex files

2 participants