Replacing all string formatting with f-strings.

[TrevorBenson]

---

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

• Is the commit message split over multiple lines and hard-wrapped at 72 characters?
• Is the subject and message clear and concise?
• Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
• Does the commit contain a Signed-off-by: First Lastname email@example.com?
• Are any related Issues or existing PRs properly referenced via a Closes (Issue) or Resolved (PR) line?

This is mostly for checks, and tests and to allow a first review by maintainers for further discussion in #3472.

[packit-as-a-service]

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

• sudo yum install -y dnf-plugins-core on RHEL 8
• sudo dnf install -y dnf-plugins-core on Fedora
• dnf copr enable packit/sosreport-sos-3536
• And now you can install the packages.

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

[TurboTurtle]

A lot of this looks good. Have some comments that follow; going solely by comments-to-code ratio it's quite a low potential miss rate though - happy to see that semi-automated conversions are mostly reliable.

I'll be honest I only got through about half of the files at this on this first look before I started to go cross-eyed. Will take another look at this tomorrow or so.

[TurboTurtle]

Ditto.

[TrevorBenson]

A lot of this looks good. Have some comments that follow; going solely by comments-to-code ratio it's quite a low potential miss rate though - happy to see that semi-automated conversions are mostly reliable.

I'll be honest I only got through about half of the files at this on this first look before I started to go cross-eyed. Will take another look at this tomorrow or so.

Yeah, mostly. Black didn't do the f-string conversion as I used another tool which I used for static analysis, and is generally very good for replacing formatting and concatenation with f-string. At least for the low-hanging fruit. Black was only for the E501 line length fixes since the first tool is great, but sometimes fails to achieve the intended line lengths.

I manually went through the diff of every file after to restore contribution style guidelines, mostly for nested lists but a handful of other idiosyncrasies that stood out on quick pass. I'll add your comments to my checklist and look for additional unintended changes or style breakage.

I'll mark this as Draft again until I do the manual review and look back through for the unfixed string formatting. Most look to be multiline strings or other variations the first tool skipped, I had that underway last night, but was trying to make it a single commit with the final changes, instead of many little commits.

[TrevorBenson]

I went through each file manually and reviewed its existing styles/syntax. I went with a previous line/PR wins methodology. Meaning if it had a specific style, even if that diverged from the style guide or earlier comments, it wins out as being reviewed and approved previously. This should keep the changes as minimal as possible to the existing code, and keep the file diffs to a minimum number of changed or new lines

There are of course a few cases where this changed.

• If it was a single string and it was shortened enough to fit on a single line after removing % string formatting, then it may have been shortened.
  • Nested lists that fit on a single line should still maintain the style guide preference.
• In rare instances a new variable was created.
  • This should be limited to only regex becoming a f-string regex. The variable was introduced to maintain the readability of the regex statement as well as keep them from breaking 80 characters and either requiring the regex to be spread over multiple lines or have a # flake8: noqa applied to avoid flake8 check failures. I felt this was the best overall solution since regex are inherently complex strings I didn't want f-string implementation to increase it and reduce their readability.
• This only applies to ./sos currently. I had a full update for ./tests but it led to ~5 failures. It wasn't immediately obvious where/how it was breaking anything when investigating. I rebased and dropped those commits for the time being.

Anything that still has a % string formatting is up for discussion, but if left in place it either:

1. Was an elegant or complex implementation and seemed worse to replace it without discussion.
2. Required more investigation to better understand the implementation and to find an adequate/acceptable replacement.

[TrevorBenson]

I restored the f-strings for the ./tests path after diagnosing the test breakage to be part of rpm.py. I thought the nested f-string formatting should have been equivalent to the original nested % string formatting. However, I must have overlooked something.

I'll count this as one of the cases that is now up for discussion. Flake8 returns no issues on the ./sos path and Pylint C0209 has zero results for both ./sos and ./tests paths.

@TurboTurtle I believe this is ready for discussion on the build failures since some RPMs appear to build ok, but not packit. I'll go ahead and mark it as ready for review.

[TrevorBenson]

@TurboTurtle @pmoravec Any suggestions on how I should diagnose the packit builds failing while non packit RPM builds appear successful?

[TurboTurtle]

@TurboTurtle @pmoravec Any suggestions on how I should diagnose the packit builds failing while non packit RPM builds appear successful?

The packit builds are passing. The cirrus jobs are failing due to avocado not being able to tell that the tests are....well, tests. This typically happens when there is something within the test class that prevents python from being able to import the class. Nothing immediately jumps out at me in the diffs, so I'd need to dig into the updated test code a bit more to figure out what's going on here.

[TrevorBenson]

@TurboTurtle @pmoravec Any suggestions on how I should diagnose the packit builds failing while non packit RPM builds appear successful?

The packit builds are passing. The cirrus jobs are failing due to avocado not being able to tell that the tests are....well, tests. This typically happens when there is something within the test class that prevents python from being able to import the class. Nothing immediately jumps out at me in the diffs, so I'd need to dig into the updated test code a bit more to figure out what's going on here.

I must have had an old check open via cirrus when I found the error for packit? If they are building fine then I'll review the tests again. Everything was done by hand/manually. However, I did that with the rpm plugin as well and there was something that expected the current format I did not account for there as well.