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

Fix @doc generation for :global attribute type (fixes #2709) #2765

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 16 additions & 16 deletions lib/phoenix_component/declarative.ex
Original file line number Diff line number Diff line change
Expand Up @@ -817,7 +817,7 @@ defmodule Phoenix.Component.Declarative do
defp build_attrs_docs(attrs) do
[
"## Attributes\n",
for attr <- attrs, attr.doc != false, attr.type != :global, into: [] do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were skipping proper generation for :global here...

Copy link
Member

Choose a reason for hiding this comment

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

I think it was written in this style so the :global attribute always comes last. Should we revert this particular change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That does make sense. I'll revert that part, then and then we can follow up on performance not potentially being enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I skipped global in the first for pass, then did an enum find at the end for the global. This means that only one global actually gets docs generated. I'm not sure if multiple globals should be supported?

It also means potentially 2 passes through the attrs list so not as performant as it technically could be, but trying to do it all in one pass means a reduce or some other less clear alternative.

for attr <- attrs, attr.doc != false and attr.type != :global do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, @josevalim i didn't realise I switched from a "multiple filters" approach in the comprehension and went with and. I see you switched it to && but should it be reverted back to multiple filters?

Copy link
Member

Choose a reason for hiding this comment

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

It is all the same!

[
"\n* ",
build_attr_name(attr),
Expand All @@ -828,10 +828,10 @@ defmodule Phoenix.Component.Declarative do
build_attr_values_or_examples(attr)
]
end,
if Enum.any?(attrs, &(&1.type == :global)) do
"\n* Global attributes are accepted."
else
""
# global always goes at the end
case Enum.find(attrs, &(&1.type === :global)) do
nil -> []
attr -> build_attr_doc_and_default(attr, " ")
end
Comment on lines +831 to 835
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the inline comment too much? I know code should be self-documenting, and a new test case ensures global goes last, so I can remove that part.

]
end
Expand Down Expand Up @@ -904,17 +904,17 @@ defmodule Phoenix.Component.Declarative do
end

defp build_attr_doc_and_default(%{doc: doc, type: :global, opts: opts}, indent) do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

But, clearly, at some point, we intended to call this function for the global type.

case Keyword.fetch(opts, :include) do
{:ok, [_ | _] = inc} ->
if doc do
[build_doc(doc, indent, true), "Supports all globals plus: ", build_literal(inc), "."]
else
["Supports all globals plus: ", build_literal(inc), "."]
end

_ ->
if doc, do: [build_doc(doc, indent, false)], else: []
end
[
"\n* Global attributes are accepted.",
if(doc, do: [" ", build_doc(doc, indent, false)], else: []),
case Keyword.get(opts, :include) do
inc when is_list(inc) and inc != [] ->
[" ", "Supports all globals plus:", " ", build_literal(inc), "."]

_ ->
[]
Comment on lines +908 to +915
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We start off with the generic line, then maybe add docs, then maybe add includes.

Copy link
Member

Choose a reason for hiding this comment

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

Perfect ❤️

end
]
end

defp build_attr_doc_and_default(%{doc: doc, opts: opts}, indent) do
Expand Down
23 changes: 22 additions & 1 deletion test/phoenix_component/declarative_assigns_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -867,6 +867,27 @@ defmodule Phoenix.ComponentDeclarativeAssignsTest do

* Global attributes are accepted.
""",
fun_attr_global_doc_include: """
## Attributes

* Global attributes are accepted. These are passed to the inner input field. Supports all globals plus: `["value"]`.
""",
fun_attr_global_doc: """
## Attributes

* Global attributes are accepted. These are passed to the inner input field.
""",
fun_attr_global_include: """
## Attributes

* Global attributes are accepted. Supports all globals plus: `["value"]`.
""",
fun_attr_global_and_regular: """
## Attributes

* `name` (`:string`) - The form input name.
* Global attributes are accepted. These are passed to the inner input field.
""",
fun_attr_struct: """
## Attributes

Expand Down Expand Up @@ -1020,7 +1041,7 @@ defmodule Phoenix.ComponentDeclarativeAssignsTest do
assert Enum.find_value(abstract_code, fn
{:function, line, :fun_doc_false, 1, _} -> line
_ -> nil
end) == 105
end) == 118
end

test "does not override signature of Elixir functions" do
Expand Down
13 changes: 13 additions & 0 deletions test/support/live_views/components.ex
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,19 @@ defmodule Phoenix.LiveViewTest.FunctionComponentWithAttrs do
attr :attr, :global
def fun_attr_global(assigns), do: ~H[]

attr :rest, :global, doc: "These are passed to the inner input field"
def fun_attr_global_doc(assigns), do: ~H[]

attr :rest, :global, doc: "These are passed to the inner input field", include: ~w(value)
def fun_attr_global_doc_include(assigns), do: ~H[]

attr :rest, :global, include: ~w(value)
def fun_attr_global_include(assigns), do: ~H[]

attr :name, :string, doc: "The form input name"
attr :rest, :global, doc: "These are passed to the inner input field"
def fun_attr_global_and_regular(assigns), do: ~H[]

attr :attr, Struct
def fun_attr_struct(assigns), do: ~H[]

Expand Down
Loading