format sigils using applicable plugins#37
format sigils using applicable plugins#37superhawk610 wants to merge 2 commits intoremoteoss:mainfrom
Conversation
See https://github.com/elixir-lang/elixir/blob/d67cb9634baba80e61188da5a688c9b127f94038/lib/mix/lib/mix/tasks/format.ex#L337 for corresponding implementation in `mix format`.
JesseHerrick
left a comment
JesseHerrick
left a comment
There was a problem hiding this comment.
Hi @superhawk610, thank you very much for your contribution! I just have one note about unifying things here.
| Process.group_leader(self(), old_gl) | ||
| end | ||
| else | ||
| sigils = |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This should avoid polluting stdout with any exception messages.
2 participants
Closes #36.
This PR modifies the
.ex/.exsformat process to include plugins that are capable of formatting sigil contents, e.g.Phoenix.LiveView.HTMLFormatterfor the~Hsigil.See https://github.com/elixir-lang/elixir/blob/d67cb9634baba80e61188da5a688c9b127f94038/lib/mix/lib/mix/tasks/format.ex#L337 for corresponding implementation in
mix format.