Skip to content

Refactor render_html_list #3

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.

Sign up for GitHub

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

Closed
klnsv wants to merge 2 commits into from
Closed

Refactor render_html_list #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
01cc752
Refactor render_html_list
klnsv Nov 24, 2025
11b117c
WIP: Removed skip_wrapper and created a parent renderer
klnsv Nov 25, 2025
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
100 changes: 59 additions & 41 deletions src/http/commands/help/presenter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,46 +70,66 @@ def initialize(manifest)
self.manifest = manifest
end

def render_html_list(data, skip_wrapper: false)
Copy link
Contributor

@azimux azimux Nov 25, 2025

Choose a reason for hiding this comment

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

we probably don't need to rename the method unless it's doing something different than before and that the new name communicates what its doing from the outside. If we rename it but keep the method maybe #render_html is a better name for a public method that decouples it from _list.

def render_html_parent(data)
# This function is the parent function of render_html_list just to eliminate the skip_wrapper mechanism.
# This function eliminates the need to use <ul> tags in the render_html_list function calls.
# Note that we are making some calls on render_html_list function but eliminating the need for skip_wrapper.
Copy link
Contributor

@azimux azimux Nov 25, 2025

Choose a reason for hiding this comment

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

We should probably put the comment on top of the method not inside it. Also we should call it "method" instead of "function" although in this case it might actually be more function-like.

Actually I'd be inclined to skip the comment. The skip_wrapper concept is gone so it's not helpful to mention it in a comment. If this is just a comment meant for PR review, the comment could just be made in the PR instead of in the code. Also, it mentions <ul> which is an implementation detail that could change at some point. Probably the easiest way in this case to be forward-compatible with various changes would be to delete the comment.

html = ""
case data
when ::Hash
html << "<ul>"
data.each do |key,value|
html << "<li>"
Copy link
Contributor

@azimux azimux Nov 25, 2025

Choose a reason for hiding this comment

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

nice, yeah this helps I think

html << "<strong>#{key}: </strong>"
html << foobara_reference_link(value.error)
html << "</li>"
end
html << "</ul>"
when Manifest::Attributes
html << "<ul>"
data.relevant_manifest.each_pair do |key, value|
if key.to_s == "type"
next
end
if key.to_s == "element_type_declarations"
key = :attributes
value = data.attribute_declarations
end
html << "<li>"
html << "<strong>#{key}: </strong>"
html << "<ul>"
html << render_html_list(value)
html << "</ul>"
html << "</li>"
end
html << "</ul>"
when Manifest::TypeDeclaration
manifest = data.relevant_manifest
if manifest.is_a?(::Symbol)
html << "<strong>Result Type: </strong>"
html << foobara_reference_link(data.to_type)
end

end
html
end

def render_html_list(data)
Copy link
Contributor

@azimux azimux Nov 25, 2025

Choose a reason for hiding this comment

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

Hmmm I'm kind of surprised to see two methods. That feels like it sort of negates the benefit of removing a flag that controls behavior. I should compare the two methods to see how they differ but I'm nervous about splitting it into two methods instead of having a flag. I'll try to see what the differences are and see if there's something we can do to combine them into one method again but without the flag.

html = ""
Copy link
Contributor

@azimux azimux Nov 25, 2025

Choose a reason for hiding this comment

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

something I think we need to start doing for Ruby 4 compatibility (I'm not 100% sure) is using the unary + operator for non-frozen string literals. So this should actually be html = +"" since we plan to mutate it. This happens in many places throughout the code though so it could be its own issue to find and fix these. But we may as well use the @+ operator whenever adding new code that wants a mutable string literal instead of a frozen string literal.

case data
when ::Hash
html << "<ul>" unless skip_wrapper
data.each do |key, value|
html << "<li>#{key}"
html << "<li><strong>#{key} </strong>"
html << "<ul>"
html << render_html_list(value, skip_wrapper: true)
html << render_html_list(value)
html << "</ul>"
html << "</li>"
end
html << "</ul>" unless skip_wrapper
when ::Array
html << "<ul>" unless skip_wrapper
data.each do |item|
html << render_html_list(item)
end
html << "</ul>" unless skip_wrapper
when Manifest::Attributes
html << "<ul>" unless skip_wrapper
data.relevant_manifest.each_pair do |key, value|
if key.to_s == "type"
next
end

if key.to_s == "element_type_declarations"
key = :attributes
value = data.attribute_declarations
end
html << "<li>#{key}"
html << "<ul>"
html << render_html_list(value, skip_wrapper: true)
html << "</ul>"
html << "</li>"
end
html << "</ul>" unless skip_wrapper
when Manifest::Array
html << "<ul>" unless skip_wrapper
data.relevant_manifest.each_pair do |key, value|
next if key == :element_type_declaration

Expand All @@ -118,43 +138,41 @@ def render_html_list(data, skip_wrapper: false)
end
html << "<li>#{key}"
html << "<ul>"
html << render_html_list(value, skip_wrapper: true)
html << render_html_list(value)
html << "</ul>"
html << "</li>"
end
html << render_html_list({ element_type: data.element_type }, skip_wrapper: true)
html << "</ul>" unless skip_wrapper
html << render_html_list({ element_type: data.element_type })
when Manifest::TypeDeclaration
manifest = data.relevant_manifest

if manifest.is_a?(::Symbol)
html << "<li>"
html << "<li><strong>type: </strong>"
html << foobara_reference_link(data.to_type)
html << "</li>"
else
html << "<ul>" unless skip_wrapper
data.relevant_manifest.each_pair do |key, value|
if key.to_s == "type"
value = root_manifest.lookup_path(key, value)
end
html << "<li>#{key}"
html << "<ul>"
html << render_html_list(value, skip_wrapper: true)
html << "</ul>"
html << "<li><strong>#{key}: </strong>"
html << render_html_list(value)
html << "</li>"
end
html << "</ul>" unless skip_wrapper
end
when Manifest::Type, Manifest::Command, Manifest::Error
html << "<li>"
when Manifest::Type
html << foobara_reference_link(data)
when Manifest::Command
html << "<li><strong>COMMAND: </strong>"
html << foobara_reference_link(data)
html << "</li>"
when Manifest::PossibleError
html << render_html_list(data.error)
when String
html << data
when true,false
html << data.to_s
else
html << "<li>#{data}</li>"
end

html
end

Expand Down
6 changes: 3 additions & 3 deletions src/http/commands/help/templates/command.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@
<p><%= description %></p>

<h2>Inputs</h2>
<%= render_html_list(inputs_type) %>
<%= render_html_parent(inputs_type) %>

<h2>Result</h2>
<%= render_html_list(result_type) %>
<%= render_html_parent(result_type) %>

<h2>Possible Errors</h2>
<%= render_html_list(possible_errors) %>
<%= render_html_parent(possible_errors) %>
Loading