sosreport / sos

A unified tool for collecting system logs and other debug information
http://sos.rtfd.org
GNU General Public License v2.0
508 stars 544 forks source link

[report] Fix html formatting for Plugin section #3781

Open jcastill opened 2 weeks ago

jcastill commented 2 weeks ago

After moving from .format() to f-strings via 57d21346b, the formatting for Plugin sections was lost. This patch attempts to fix this while still using f-strings.

Related: #3780


Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

packit-as-a-service[bot] commented 2 weeks ago

Congratulations! One of the builds has completed. :champagne:

You can install the built RPMs by following these steps:

Please note that the RPMs should be used only in a testing environment.

jcastill commented 2 weeks ago

Two thoughts, no particular preference on either:

1. We are only using these in 2 places, we could simply do the f-string in-line instead of breaking it out into a function.  We haven't really made any _major_ changes to how the HTML reports are generated for in a very long time so I doubt we're going to be expanding the use of these any time soon.

Fair enough. The problem I tried to solve was for example with PLUGLISTITEM, we have a definition for plain text and another one for html, and while setting the variable via f-strings was a bit tricky. Do you know if there's a pythonic way to use f-strings with strings defined as variables like PLUGLISTITEM? Another option, perhaps, is to set a is_html boolean var, and if that's true, then add the html code, and if it's false (i.e. coming from the plain text report) then just print the var. Something like:

str_plugin = [ f"{value}" if not is_html else f'<td><a href="#{value}">{value}</a></td>\n' ]

2. If we keep it as a function, then we don't use camelCase in sos, and PEP8 [explicitly discourages it](https://peps.python.org/pep-0008/#function-and-variable-names)

Sorry, it seems that old habits die hard...

jcastill commented 1 week ago

Fair enough. The problem I tried to solve was for example with PLUGLISTITEM, we have a definition for plain text and another one for html, and while setting the variable via f-strings was a bit tricky. Do you know if there's a pythonic way to use f-strings with strings defined as variables like PLUGLISTITEM? Another option, perhaps, is to set a is_html boolean var, and if that's true, then add the html code, and if it's false (i.e. coming from the plain text report) then just print the var. Something like:

str_plugin = [ f"{value}" if not is_html else f'<td><a href="#{value}">{value}</a></td>\n' ]

Replying to myself: List comprehension won't be the best approach here, because we are concatenating the result. Perhaps an old fashioned if/then could be simpler.

pmoravec commented 1 week ago

As we use the %(name)s syntax else-where in reporting.py, what about patch:

--- a/sos/report/reporting.py
+++ b/sos/report/reporting.py
@@ -136,11 +136,11 @@ class PlainTextReport:
     ALERT = "  ! %s"
     NOTE = "  * %s"
     PLUGLISTHEADER = "Loaded Plugins:"
-    PLUGLISTITEM = "  {name}"
+    PLUGLISTITEM = "  %(name)s"
     PLUGLISTSEP = "\n"
     PLUGLISTMAXITEMS = 5
     PLUGLISTFOOTER = ""
-    PLUGINFORMAT = "{name}"
+    PLUGINFORMAT = "%(name)s"
     PLUGDIVIDER = "=" * 72

     subsections = (
@@ -168,7 +168,7 @@ class PlainTextReport:
         i = 0
         plugcount = len(self.report_data)
         for section_name, _ in self.report_data:
-            line += f"  {section_name}"
+            line += self.PLUGLISTITEM % {'name': section_name }
             i += 1
             if (i % self.PLUGLISTMAXITEMS == 0) and (i < plugcount):
                 line += self.PLUGLISTSEP
@@ -177,7 +177,7 @@ class PlainTextReport:

         for section_name, section_contents in self.report_data:
             line_buf.append(self.PLUGDIVIDER)
-            line_buf.append(f"{section_name}")
+            line_buf.append(self.PLUGINFORMAT % {'name' : section_name})
             for type_, format_, header, footer in self.subsections:
                 self.process_subsection(section_contents, type_.ADDS_TO,
                                         header, format_, footer)
@@ -224,11 +224,11 @@ class HTMLReport(PlainTextReport):
     ALERT = "<li>%s</li>"
     NOTE = "<li>%s</li>"
     PLUGLISTHEADER = "<h3>Loaded Plugins:</h3><table><tr>"
-    PLUGLISTITEM = '<td><a href="#{name}">{name}</a></td>\n'
+    PLUGLISTITEM = '<td><a href="#%(name)s">%(name)s</a></td>\n'
     PLUGLISTSEP = "</tr>\n<tr>"
     PLUGLISTMAXITEMS = 5
     PLUGLISTFOOTER = "</tr></table>"
-    PLUGINFORMAT = '<h2 id="{name}">Plugin <em>{name}</em></h2>'
+    PLUGINFORMAT = '<h2 id="%(name)s">Plugin <em>%(name)s</em></h2>'
     PLUGDIVIDER = "<hr/>\n"

     subsections = (
jcastill commented 1 week ago

@pmoravec I think this may work as well. We have now a number of different possible solutions. Which one should I send?

pmoravec commented 1 week ago

No strong preference. Just the %s-strings are quite obsolete (citation needed) and we might rather use .format() syntax..?