[global] Replace use of .format() with f-strings
#3505
Conversation
As part of the project's effort to modernize on f-strings, this commit replaces all usage of .format() with f-string equivalents. The vast majority of these are in-place syntax changes, but a limited number change the order of list items or some formatting tricks in order to appease PEP8. None of these conversions change the underlying logic of the flows they appear in. Related: sosreport#3472 discussion Signed-off-by: Jake Hunsaker <jacob.r.hunsaker@gmail.com>
|
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. |
|
Note: I'll be holding off on merging this until after 4.7 gets cut. While these are simple changes, there's enough of them that I'd prefer we have a full release testing cycle on them as opposed to ~2 weeks. |
|
|
||
| cmd = ( | ||
| f"PGPASSWORD={self.conf['ENGINE_DB_PASSWORD']} /usr/sbin/sos " | ||
| f"report --name=postgresql --batch -o postgresql {sos_opt}" |
There was a problem hiding this comment.
We can use plugin variable here as well - but it might be ridiculous..?
There was a problem hiding this comment.
True, I can update that.
| f"/var/log/{self.apachepkg}*/error_log*", | ||
| f"/etc/{self.apachepkg}*/conf/", | ||
| f"/etc/{self.apachepkg}*/conf.d/", | ||
| f"/var/log/{self.apachepkg}*/katello-reverse-proxy_access_ssl.log*" |
There was a problem hiding this comment.
Nitpick: it makes sense to have similar copyspecs altogether, so put katello-reverse-proxy_access_ssl.log* just below the other katello-reverse-proxy* one.
Ditto in foreman_proxy plugin.
There was a problem hiding this comment.
This is due to PEP8 as referenced in the commit message - the only way I found to keep from line-breaking due to going over 79 characters was to be cheeky and stick it at the end without a comma ;-). The other option would involve more use of *, but I'm not familiar with all the log naming here and didn't want to risk messing up some collections.
There was a problem hiding this comment.
Another option is disable PEP8 and pylint warnings on specific lines such as:
LONG LINE over 79 chars here # noqa, pylint: disable=C0301
FWIW, I absolutely love trailing comma at the end (easy to reorder and/or sort stuff ;-)
|
This implicitly drop support for Python 3.5 and older. |
With releasing 4.0, we claimed Breaking compatibility with Python 3.5 or older is / would be a point. We should at least state it somewhere. In fact, I expect |
|
The thing is we have been using fstrings for a while now and older systems would already not work. We have a situation where 16.04 (xenial) no longer works and we can't backport sos to that distro for this reason. So there we are stuck with 3.9.1. But that is now on security maintenance anyhow, so no urgency on getting new sos. But the rest for us works, and we don't have this issue. So even though 18.04 (bionic) is now also security maintenance as of last year we are still keen to get that going still. As long as it keeps working there we will be continuing to support it. Ultimately, I'm keen to go in this way, and after reading a few articles fstrings seem faster and more intuitive, so all up for it. BTW, I found a python module called I was also looking at |
That's true.
Not sure if that's correct. I believe only plugins that currently use f-strings would fail and rest of the data would be collected. With this PR, we are introducing more f-strings by converting
Agree. If that's stated as a minimal requirement, that's fine. It'd be increasingly less of an issue as time goes by anyway. I just wanted to get it clarified and explicitly stated so. |
I haven't used flynt before - I can look into it. |
At a high level - I'd love to move forward on our minimum python version. However several of our downstreams still support (or rather, require the use of) python 3.6 which is why we haven't pressed forward with making the minimum version something like 3.9 (still EOL'd I believe?) or more recent 3.11 or 3.12. I know of two specific ones from major contributing companies - RHEL 8 and Ubuntu 18.04, so we'll need to keep 3.6 around until we no longer need to worry about compatibility there. That doesn't mean we keep 3.6 until these releases are no longer supported at all, though. RHEL 7 still has commercial support, but we don't develop for that target. We have the
Looks interesting. I'll take a look, even if it doesn't work perfectly for us it likely still does a lot of the heavy lifting.
We talked about black before, and the short and sweet of it is that while it does some stuff well, the stuff it doesn't do well, or the formatting it does that we don't like, we really don't like. The off the top of my head example is indention of long strings as parameters to method calls. With the way black does its line breaks it's near impossible to distinguish at a glance what line is a continuation of the previous one, and which is a new parameter. |
We use it on more places, like: But if there is some particular OS with pre-3.6 version that can run the current upstream sos, that would be an argument to pause with this PR, for sure.
Indeed, both OSes use Python 3.6.8 that we will need to support in sos upstream for some/many(?) years. Gladly f-strings were introduced in 3.6, so at least these two distro versions wont be negatively affected by this PR. They will "just" conserve our attempts to move minimal requirements beyond 3.6. |
Right, but my main point was that we don't need to necessarily hold upstream for as long as those versions are in use. Again, for RHEL 7 we stopped developing for it even though it is still used/supported today. We made the cut after the final minor release, and left the previous version in the I think we should do the same here - find a reasonable cut off date for Red Hat and Canonical and mark the current state in a/another legacy branch - otherwise we'll be on 3.6 in 2030 which helps no one. |
I'll circle back on this in the near future. |
|
From an Ubuntu standpoint, we are currently working on pushing either Currently Ubuntu Hope this makes sense. |
Oof. I was really hoping the next Ubuntu release we had to care about after 18.04 had 3.9 at least as that's what EL9 runs with. According to the python version timeline, 3.8 goes EOL towards the end of this year with 3.9 going EOL the end of 2025. Tell you what, let's continue this in a github discussion that I'll open later today. For now, the conversation has definitely evolved beyond the original point of this PR. Speaking of which, if there are no objections, I'll merge this shortly since 4.7.0 was cut yesterday. |
As part of the project's effort to modernize on f-strings, this commit replaces all usage of .format() with f-string equivalents. The vast majority of these are in-place syntax changes, but a limited number change the order of list items or some formatting tricks in order to appease PEP8.
None of these conversions change the underlying logic of the flows they appear in.
Related: #3472 discussion
Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines