-
Notifications
You must be signed in to change notification settings - Fork 935
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
Changes from all commits
eedae24
54354a6
d55386a
0b1914b
a4a4f32
c49660f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
for attr <- attrs, attr.doc != false and attr.type != :global do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is all the same! |
||
[ | ||
"\n* ", | ||
build_attr_name(attr), | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -904,17 +904,17 @@ defmodule Phoenix.Component.Declarative do | |
end | ||
|
||
defp build_attr_doc_and_default(%{doc: doc, type: :global, opts: opts}, indent) do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
There was a problem hiding this comment.
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...There was a problem hiding this comment.
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?There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.