Run code formatter on typespecs #800
Conversation
|
I did some random testing on Ecto & Elixir codebases and looks pretty good, the only minor regression I've seen is so far is: before:
(https://hexdocs.pm/elixir/master/File.html#t:mode/0) after:
|
|
How can we fix this regression? |
|
it looks like existing code that was breaking on |
|
Ebert has finished reviewing this Pull Request and has found:
You can see more details about this review at https://ebertapp.io/github/elixir-lang/ex_doc/pulls/800. |
![sourcelevel-bot[bot]](https://avatars.githubusercontent.com/in/597?s=60&v=4){kind=small}
| end | ||
| end | ||
|
|
||
| # TODO: remove when we require Elixir v1.6+ |
|
|
||
| if formatter_available?() do | ||
| string | ||
| |> Code.format_string!(line_length: @line_length) |
There was a problem hiding this comment.
for line_length: 70 I noticed it caused churn in a few places, e.g.: https://hexdocs.pm/elixir/master/Access.html#elem/1 vs:
It might be better to stick to default line length on formatter (which then breaks other tests and I didn't want to necessarily add more if formatter_available? clauses or adjust tests 😅 )
|
@josevalim fixed 🎉 :
|
|
|
||
| assert Autolink.typespec(quote(do: (really_long_name_that_will_trigger_multiple_line_breaks(1) :: bar | baz)), [], []) == | ||
| ~s[really_long_name_that_will_trigger_multiple_line_breaks(1) ::\n bar |\n baz] | ||
| if formatter_available?() do |
There was a problem hiding this comment.
another option to manage these tests is to have:
test "add new lines on |" do
...
end
@tag :formatter
test "add new lines on | with formatter" do
...
end
@tag :no_formatter
test "add new lines on | without formatter" do
...
endand then configure proper exclude in test_helper.exs
There was a problem hiding this comment.
I like the tag approach the most.
|
|
||
| def typespec({:when, _, [{:::, _, [left, {:|, _, _} = center]}, right]} = ast, typespecs, aliases, lib_dirs) do | ||
| defp format_typespec({:when, _, [{:::, _, [left, {:|, _, _} = center]}, right]} = ast, typespecs, aliases, lib_dirs) do | ||
| if short_typespec?(ast) do |
There was a problem hiding this comment.
I don't think we need this? We should let the formatter handle it all, no?
There was a problem hiding this comment.
this is left as is for when formatter is not available. When we require 1.6 I'd be happy to 🔥 this
There was a problem hiding this comment.
Just to clarify, format_typespec, short_typespec, format_typespec_with_new_line, normalize_left will all be gone when we depend on 1.6
There was a problem hiding this comment.
Oh, sorry. I got confused. I thought format_typespecused the formatter (since it says format in the name) and I thought that typespec_to_string used Macro.to_string since it has to_string. Should we flip the names?
There was a problem hiding this comment.
doh, good call! All fixed now
| defp short_typespec?(ast) do | ||
| byte_size(Macro.to_string(ast)) <= 70 | ||
| defp put_placeholder(form, string, placeholders) do | ||
| id = zero_pad(map_size(placeholders), 3) |
There was a problem hiding this comment.
I don't think we need to pad, no?
There was a problem hiding this comment.
yup, not anymore as there's always a trailing _
|
❤️ 💚 💙 💛 💜 |
Closes #799