Ruff formatter [v2]: remove flake8/black/isort/bandit and format to ruff defaults #2844
Conversation
…hat Ruff had missed
Missed one in previous commit
|
Note: Type annotations using the Python 3.9/3.10 new syntax should work with 3.8 too, thanks to the future imports that Ruff added to each module. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2844 +/- ##
=============================================
+ Coverage 88.387% 88.481% +0.094%
=============================================
Files 92 92
Lines 7156 7145 -11
Branches 1228 1223 -5
=============================================
- Hits 6325 6322 -3
+ Misses 575 571 -4
+ Partials 256 252 -4
☔ View full report in Codecov by Sentry. |
…s (they will timeout anyway by third).
Tronic
changed the title
1500 lines less code, far improved formatting of things that were split in unreadable ways. All typing upgraded to modern format. Four separate tools costing expensive CI runs replaced with one that runs much faster (with setting up that one CI job still takes about 40 seconds). Including examples, guide and tests in CI too. I believe these are benefits enough to justify the changes of this PR [v2]. But I have also provided the other PR #2843 in case such a large rewrite of code is not wanted just yet. Ruff notably can keep almost all Black formatting as it were, as demonstrated in that PR. Notably prominent projects like FastAPI and Pydantic immediately switched their tooling to Ruff with this new release that supports full formatting. |
|
Ruff targets all Sanic's supported Python versions in one run, if we add to pyproject.toml: [project]
requires-python = ">=3.8"Thus it also doesn't need separate CI runs on each version, as was previously done with bandit (security) presumably because that tool only checks for the Python version it runs on. |
There was a problem hiding this comment.
95%+ of the changes are related to formatting and syntax updates. I am perfectly fine with updating Union[] and Optional[] to their pipe syntax as I think it is cleaner and easier to read. I'm also ok with replacing dict and set explicits with implicits. I also am fine with the default line length being moved to 88 chars, though I think that is a project level decision that the entire core dev team should weigh in on. I'm sure there will be opinions.
This has my tentative approval and once the line length issue has been settled, I'll add explicit approval if the 88 char line length is accepted.
| @@ -10,7 +9,6 @@ | |||
| import os | |||
| import sys | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Looks like ruff moves from 2 line breaks between imports and code to one. I don't know if I actually like that change because it provides a better visual break, though with any modern editor, code highlighting generally makes this fine. If nobody else finds the 2lb useful, I'm fine with moving forward with just the one.
There was a problem hiding this comment.
Sanic was somewhat 50%/50% one or two lines after imports. The guide and other parts not included in automatic formatting had one line while the main codebase had two. I opted for Ruff default, which is one line, to unify everything and to avoid extra space that might not be wanted in guide.
We can provide different configuration for different parts of Sanic code if so desired. I personally don't care about the specific number of empty lines there (or the various other positions where two lines might be inserted).
| @@ -14,7 +14,7 @@ def _apply_exception_handler(self, handler: FutureException): | |||
| def exception( | |||
| self, | |||
| *exceptions: Union[Type[Exception], List[Type[Exception]]], | |||
| apply: bool = True | |||
| apply: bool = True, | |||
There was a problem hiding this comment.
Nitpicking that I don't think a trailing comma belongs before the closing parenthesis.
There was a problem hiding this comment.
It is a fairly common to put comma even after the last item in expanded (multi line) format, so that adding more entries does not require adding a comma on the previous line. Black apparently also does this most of the time... And there appears to be no configuration for it.
This is one of the things that Ruff apparently intentionally deviates from Black in some yet not clearly specified cases. https://docs.astral.sh/ruff/formatter/black/#trailing-commas-are-inserted-when-expanding-a-function-definition-with-a-single-argument
| or platform.system() != "Linux", | ||
| reason="Not testable with current client", | ||
| ) | ||
| def test_keep_alive_client_timeout(): |
There was a problem hiding this comment.
We're axing this test completely. Is it just because we don't find the test useful or is there some other background?
There was a problem hiding this comment.
The test was hanging for a long time. It tests a timeout of httpx, which is essentially a disconnection on Sanic's side, and is not particularly meaningful to test.
…terwards because ruff format warns of possible problems with it.
…s to [project] in pyproject.toml)
This is a Python 3.10 feature. |
Supported fine on Python 3.8 and 3.9 as well by |
|
Closing this for now, but let's come back and revisit some of this after LTS. |
|
Keeping the branch to revisit this after LTS... It includes plenty of manual modification for tests and typing. |
|
I won't have time to get back to this. A lot of changes have piled up after this branch, so deleting it anyway. |
This is a more radical fork of my other Ruff PR #2843 which already had a fairly large diff. This one strips away the old linters and doesn't attempt to follow their style anymore, so the diff keeps growing...
Both PRs are available for review and discussion. Have a look and state which one if either should be merged.