Testing using pylint to detect f-strings

[arif-ali]

One of the common things that we are asking people to do is now use f-strings for all the new PRs. So, thought we should find something that can actually detect this for us, and our CI to detect this and alert straight away

Now, pylint can detect these for us, so that we don't forget :)

Now, trying this on our project gives us a significant amount of lint issues. https://dpaste.com/2F7AFCCDY. I just ran a pylint sos in the current state

My thought here is, that we disable most of these at first, convert all the f-strigs, and at least check for these. Then, maybe, we could start adding the extra tests and improve the ilnting.

Thoughts?

[TurboTurtle]

Yeah, the shear volume of non-f-string formatting in the project is why I haven't thrown anything into the CI jobs for it. We have ~10 years of string substitution and .format() built up that makes it, in my opinion, untenable to turn it on just yet.

Perhaps if it could be tuned to new additions we could add it rather quickly, but I think we need to get through at least the bulk of the existing conversions first.

[TurboTurtle]

After thinking on this some more, I'm of the opinion we're just going to have to make a concerted effort in the next release cycle or two to do these conversions in large batches. I don't see a way to piecemeal this out and have any substantial movement in any reasonable amount of time if we want to formally make a check on f-strings going forward.

[arif-ali]

yeah, agreed. when I ran that first pylint with only f-strings there were too many to look at all in one go. there's many hours of work o convert everything.

I think going through these would be great, and then we can start looking at other linting issues too ;)

[TurboTurtle]

I've opened #3505 to kick this off. We used .format() not nearly as much as string substitution, or as much as I thought we did generally (though I still went cross-eyed a few times ;) ) so it was easy enough to knock it out in one go, if a bit tedious.

String substitution...we're gonna be here for a while.

[TrevorBenson]

I took a first pass at this and am hopeful the testing can vet out any issues that may have been left. I also went at it from a global perspective, which may not have been what we wanted. I believe in some cases, such as logging, the use of "%" based formatting is still suggested over f-strings.

However, most logging appears to have not been following the Pylint suggestion for logging format interpolation (W1202), instead of:

        self.log_info("initialised empty FileCacheArchive at '%s'" % (self._archive_root,))

using:

        self.log_info("initialised empty FileCacheArchive at '%s'", self._archive_root)

So the strings were being interpolated whether or not the messages were being emitted.

Therefore moving them to f-strings should be an "apples to apples" conversion, meaning I believe that f-strings will also be interpolated regardless of the log message being emitted so there should be a similar performance observed between the main and the branch. However, this is one case to potentially consider not using f-strings and implementing logging format interpolation. I am not sure how beneficial this would be in the end, so my first pass simply changes everything to f-strings anyway, and if we want to try it out I suggest we handle this in a separate branch/PR after finishing an f-string conversion and passing all tests and checks.

I opened a draft PR for discussion. My first commit implements the f-strings, and the second commit attempts to restore flake8 E501 compliance. Since I believe E501 should be the only thing the f-strings swap broke I did not address other flake8 complaints.

The draft PR is #3536, since I only addressed E501 I suspect we will still see flake8 failures, my test against upstream main seemed to show there were 296 flake 8 issues already existing before I started (unless maybe I used the wrong version of flake8?)

$ flake8 . | wc -l
296

[TrevorBenson]

There are more in the new branch:

$ flake8 . | wc -l
329

I suspect it should mostly be an inconsistent number of blank lines. I can take a second pass to address those if everything else looks acceptable.

It would have been less work to format via trunk linters, but that would have changed much more than line lengths after the f-string changes. So to avoid a massive format change also be introduced I wrote a pipeline to identify each file the commit originally changed w/ f-string implementation, used flake8 to identify the first E501, extract the line, format it with black --preview -l 79 ./tmp.file then replace the original line per file and keep processing until the file was complete and move onto the next file.

A few items I had to do by hand, but I believe they are all valid syntax. 🤞

[TrevorBenson]

I fixed the rest of the flake8 errors and pushed another commit. I do suspect that there may be some changes to the preferred style, though. I'm going back through the contribution guidelines now to see if anything stands out. However, please don't hesitate to mention any breakage in the preferred style if you observe unwanted things in the PR.

Thanks.

[TrevorBenson]

I found a few nested lists were were single lines I am in the process of fixing. Although while going file by file in the PR view, I can also see that the method I used, which should have been limited to only f-string changes, also appears to have changed some dictionary entries that were not fstrings to use a single line when it was shorter than 80 characters combined... 😒

The primary example so far is sos/collector/transports/oc.py's get_node_pod_config() function and its manifest definition. I'll leave it for review and roll back once requested.