Refactor render_html_list #3
Conversation
|
oooo thank you! I'll spin up a rack connector for a super complex domain and poke around to see if anything looks bad |
|
Please look at the above commit, which removes the functionality of Please check this with the script that you used yesterday I am aware that there could be some edge cases.
|
|
Oof, Line coverage failed. Can I take care of that once we finalize the refactor? |
Yeah that's always totally fine. Whatever order is fine for that kind of thing. |
| self.manifest = manifest | ||
| end | ||
|
|
||
| def render_html_list(data, skip_wrapper: false) |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
| when ::Hash | ||
| html << "<ul>" | ||
| data.each do |key,value| | ||
| html << "<li>" |
There was a problem hiding this comment.
nice, yeah this helps I think
| html | ||
| end | ||
|
|
||
| def render_html_list(data) |
There was a problem hiding this comment.
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.
| end | ||
|
|
||
| def render_html_list(data) | ||
| html = "" |
There was a problem hiding this comment.
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.
|
Closing in favor of #4 |
2 participants
This PR refactors the
render_html_listfunction to minmize the usage of unnecessary<ul>and<li>tagsI tried improving the UI but it messed up with the other components of the html page so I couldn't do much about it.
Please test it out extensively with different examples and let me know if anything breaks
Thanks!!!