-
Notifications
You must be signed in to change notification settings - Fork 543
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
[report] Fix html formatting for Plugin section #3781
base: main
Are you sure you want to change the base?
Conversation
After moving from .format() to f-strings via 57d2134, the formatting for Plugin sections was lost. This patch attempts to fix this while still using f-strings. Related: sosreport#3780 Signed-off-by: Jose Castillo <[email protected]>
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
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.
Two thoughts, no particular preference on either:
-
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.
-
If we keep it as a function, then we don't use camelCase in sos, and PEP8 explicitly discourages it
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?
Sorry, it seems that old habits die hard... |
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. |
As we use the
|
@pmoravec I think this may work as well. |
No strong preference. Just the %s-strings are quite obsolete (citation needed) and we might rather use |
After moving from .format() to f-strings via
57d2134, 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