Skip to content

Refactor render_html_list #4

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

Merged
azimux merged 2 commits into from
Nov 29, 2025
Merged

Refactor render_html_list #4

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c988e32
refactor render_html_list
klnsv Nov 26, 2025
e163310
resolved errors
klnsv Nov 28, 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
44 changes: 18 additions & 26 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,28 +70,23 @@ def initialize(manifest)
self.manifest = manifest
end

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

@azimux azimux Nov 29, 2025

Choose a reason for hiding this comment

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

not related to this PR but we could change this to html = +"" to make this compatible with upcoming frozen string changes in Ruby


case data
when ::Hash
html << "<ul>" unless skip_wrapper
html << "<ul>"
data.each do |key, value|
html << "<li>#{key}"
html << "<ul>"
html << render_html_list(value, skip_wrapper: true)
html << "</ul>"
html << render_html_list(value)
html << "</li>"
end
html << "</ul>" unless skip_wrapper
html << "</ul>"
when ::Array
html << "<ul>" unless skip_wrapper
Copy link
Contributor

@azimux azimux Nov 29, 2025

Choose a reason for hiding this comment

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

I think that because we are rendering a collection here, we need the <ul> and a <li> for each child.

data.each do |item|
html << render_html_list(item)
end
html << "</ul>" unless skip_wrapper
when Manifest::Attributes
html << "<ul>" unless skip_wrapper
html << "<ul>"
data.relevant_manifest.each_pair do |key, value|
if key.to_s == "type"
next
Expand All @@ -102,57 +97,54 @@ def render_html_list(data, skip_wrapper: false)
value = data.attribute_declarations
end
html << "<li>#{key}"
html << "<ul>"
html << render_html_list(value, skip_wrapper: true)
html << "</ul>"
html << render_html_list(value)
html << "</li>"
end
html << "</ul>" unless skip_wrapper
html << "</ul>"
when Manifest::Array
html << "<ul>" unless skip_wrapper
html << "<ul>"
data.relevant_manifest.each_pair do |key, value|
next if key == :element_type_declaration

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 << render_html_list(value)
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 })
html << "</ul>"
when Manifest::TypeDeclaration
manifest = data.relevant_manifest

html << "<ul>"
if manifest.is_a?(::Symbol)
html << "<li>"
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 << render_html_list(value)
html << "</li>"
end
html << "</ul>" unless skip_wrapper
end
html << "</ul>"
when Manifest::Type, Manifest::Command, Manifest::Error
html << "<ul>"
html << "<li>"
html << foobara_reference_link(data)
html << "</li>"
html << "</ul>"
Copy link
Contributor

@azimux azimux Nov 29, 2025

Choose a reason for hiding this comment

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

Since these are not collections, I think we should not render <ul> here. Maybe this is only give the right result since ::Array is missing its <ul>s and <li>s.

Copy link
Contributor Author

@klnsv klnsv Nov 29, 2025

Choose a reason for hiding this comment

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

Ahh, yeah this might work only for ::Array I think. I added it like this for the sake of uniformity. I will commit it after testing. It will fail if we add <ul> in ::Array and the elements are something like ::Hash or Manifest::TypeDeclaration

when Manifest::PossibleError
html << render_html_list(data.error)
else
html << "<ul>"
html << "<li>#{data}</li>"
html << "</ul>"
Copy link
Contributor

@azimux azimux Nov 29, 2025

Choose a reason for hiding this comment

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

ah it kind of surprises me that we render the ul/li stuff here. But seems like it gives the right result in the browser.

Copy link
Contributor

@azimux azimux Nov 29, 2025

Choose a reason for hiding this comment

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

So thinking about this... I think we should not render ul/li here. It's just a datapoint. If we want it in a ul/li it would be the responsibility of its parent. Basically wherever we are rendering a collection we would use ul/li but wherever it's an atom we'd just render the atom (with whatever additional formatting we might want but not a single-item unordered list.)

Copy link
Contributor Author

@klnsv klnsv Nov 29, 2025

Choose a reason for hiding this comment

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

Idk maybe I'm wrong. So, the parent datatype sometimes has a collection, which are filled with elements of the same datatype as the parent. So, it fails in that case without skip wrapper

Copy link
Contributor Author

@klnsv klnsv Nov 29, 2025

Choose a reason for hiding this comment

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

Since you said the tree can be arbitrary, I thought this would be safe but also, I tried testing out what you said before making this decision and it actually required skip_wrapper. For example, there are some cases where the ::Hash has ::Hash elements as its child and it fails then. That is why I refactored this way.

end

html
Expand Down