Added config file for the Ruff Python linter and fixed additional errors #2473
Conversation
… of percent format
… `break` statement; remove the `else` and dedent its contents
…ented assignment directly
…x` over conditionally replacing with slice.
… followed by `del dict[key]`
…n privs` directly
…ontinue` statement
…rentheses for tuples in subscripts
… for `pem`, merge into a single call
…xpression with `or` operator
…licit `encoding` argument
…caping inner quotes
…assed to `re` function
…re `return` statement
…ng literal, assign to variable first
… of percent format
…side the exception class
…packing instead of concatenation
…for opening files
…o but never used
…t with multiple contexts instead of nested `with` statements
…a dict use the `keys()` method
tdulcet
force-pushed
the
python-linting
branch
from
75a71b3 to
70bf676
Compare
| @@ -530,8 +529,7 @@ def check_certificate(domain, ssl_certificate, ssl_private_key, warn_if_expiring | |||
| # should work in normal cases). | |||
| wildcard_domain = re.sub(r"^[^\.]+", "*", domain) | |||
| if domain not in certificate_names and wildcard_domain not in certificate_names: | |||
| return ("The certificate is for the wrong domain name. It is for %s." | |||
| % ", ".join(sorted(certificate_names)), None) | |||
| return ("The certificate is for the wrong domain name. It is for {}.".format(", ".join(sorted(certificate_names))), None) | |||
There was a problem hiding this comment.
Is there a reason why an f-string can't be used here?
return ("The certificate is for the wrong domain name. It is for {', '.join(sorted(certificate_names))}.", None)
There was a problem hiding this comment.
No, an f-string could of course be used here. This is just a limitation of the autofix for the UP032 rule, as it does not yet support fixes that would require handling of nested quote characters. See astral-sh/ruff#2031. These could be fixed manually, but it is obviously rather tedious, so I was just waiting for the autofix to be improved.
|
|
||
| # There is some unknown problem. Return the `openssl verify` raw output. | ||
| return ("There is a problem with the certificate.", verifyoutput.strip()) | ||
|
|
||
| # `openssl verify` returned a zero exit status so the cert is currently |
There was a problem hiding this comment.
From here to the end of the function is difficult to get through because of how github is presenting the unified diff. It appears to be okay, but maybe other eyes here would be good.
There was a problem hiding this comment.
I would recommend reviewing each of the relevant commits separately, if needed.
| @@ -223,14 +222,14 @@ class EditConf(Grammar): | |||
| EOL | |||
| ) | |||
| def value(self): | |||
| conffile = self[1] | |||
| # conffile = self[1] | |||
There was a problem hiding this comment.
The conffile var is not used here, so the linter is set to comment out code like this versus deleting it altogether?
There was a problem hiding this comment.
The autofix for the F841 rule wanted to remove the assignment, but keep the self[1] expression (Ruff does not know if it has side effects). I elected to comment it out instead, as this file does not actually run and so this variable may actually be needed if/when it is fixed.
There was a problem hiding this comment.
Okay, so I've finally gotten through the entire PR. This was a slog, and it took multiple sessions over multiple days. I left some comments and questions about a few items but overall it looks fine to me.
I think it's difficult for our small brains to digest this size of changeset. This reminds me of a phenomenon I became aware of long ago in my workplace. Basically, I realized if someone posted a 25-line PR everyone had an opinion about it. But if someone posted a 500-line PR... crickets. And ultimately the large change would sail through with little to no dissent.
Because of this I believe it would be better to keep future linter PRs to smaller bite-sized chunks, something like 5 files or 30 lines, whichever comes first. Yes, it means more PRs but I believe they would be more readily reviewed and ultimately would be safer in terms of stability and security of the product. Just my 2 cents.
|
Thanks for taking the time to review this!
Each commit only fixes a single lint rule and almost all of them are less than 5 files/30 lines. In the future, I would recommend reviewing each of those individual commit separately, as that should make it much easier to see the specific changes. Of course, this PR along with #2343 fixes all of the low hanging fruit Ruff lintier errors, so any future PRs to fix the remaining errors would likely require some refactorings and thus a larger changeset. Based on the Ruff config included in this PR, there are 520 remaining linter errors: |
SyntaxWarningstarting with Python 3.12.pyproject.tomlconfig file, to make it easier for contributors to run the linter.tools/readable_bash.pycaused by mixing tab and space indentation.As requested previously, I only fixed a single rule per commit.