Format using Ruff instead of Black. #2988
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2988 +/- ##
==========================================
- Coverage 99.63% 99.63% -0.01%
==========================================
Files 117 117
Lines 17598 17581 -17
Branches 3171 3169 -2
==========================================
- Hits 17533 17516 -17
Misses 46 46
Partials 19 19
|
There was a problem hiding this comment.
Not sure if we use them, but https://docs.astral.sh/ruff/formatter/#docstring-formatting might be nice. If we do use code in docstrings, then we'll need to check line numbers in docs are accurate afterwards.
| @@ -184,8 +213,8 @@ def gen_public_wrappers_source(file: File) -> str: | |||
|
|
|||
| generated = ["".join(header)] | |||
|
|
|||
| source = astor.code_to_ast.parse_file(file.path) | |||
| method_names = [] | |||
| source = ast.parse(file.path.read_text()) | |||
There was a problem hiding this comment.
Is there a reason why we're not using astor anymore?
If it's because we're running this through an autoformatter which should do AST -> code, then maybe we could use ast.unparse instead of astor.to_source and drop astor entirely. Though I see that ast.unparse is 3.9+ (cause type hints...) so I guess maybe that's just a # TODO: ... comment for when we drop 3.8.
There was a problem hiding this comment.
Is there a reason why we're not using astor anymore?
astor is untyped. This function is literally just a wrapper around ast.parse that opens the file. Errors in my editor annoy me.
src/trio/_tools/gen_exports.py
Outdated
| @@ -3,6 +3,9 @@ | |||
| Code generation script for class methods | |||
| to be exported as public API | |||
| """ | |||
|
|
|||
| # mypy: disable-error-code="unused-ignore" | |||
There was a problem hiding this comment.
Is mypy saying that the astor call's # type: ignore is unused? Why/why is it wrong?
PS you can # type: ignore an unused-ignore error, just explicitly provide the unused-ignore code: # type: ignore[error-code1, unused-ignore].
There was a problem hiding this comment.
Mypy doesn't care about untyped libraries; pyright strict does.
There was a problem hiding this comment.
I would much rather type ignore the specific lines that are causing issues than the entire file, because if something in our assumptions change later that will make debugging more tricky, and plus I would consider it better practice.
|
I'd like to get this PR approved and merged as soon as possible so I can base my next PR off of it. I can't really imagine anything objectionable here. |
There was a problem hiding this comment.
I doubt any content here is bad, but someone might want you to add something. (I'll probably add the TODO about ast.unparse and maybe change the # type: ignore on astor.to_source to be # type: ignore[unused-ignore] in a separate PR if you don't)
I think someone else should merge this as a final set of eyes. If you want to make the other PR now, you could rebase this to be fewer commits and we can use the "merge commit" merge here to make git history work out.
There was a problem hiding this comment.
Looks pretty good other than a few things I noted in review comments, might want to look closer at a few docstrings. Not sure I particularly like the new way ruff is formatting the type annotations, would be interested in seeing if there are configuration options for the formatter to potentially change that.
Looks pretty good overall though!
src/trio/_tools/gen_exports.py
Outdated
| @@ -3,6 +3,9 @@ | |||
| Code generation script for class methods | |||
| to be exported as public API | |||
| """ | |||
|
|
|||
| # mypy: disable-error-code="unused-ignore" | |||
There was a problem hiding this comment.
I would much rather type ignore the specific lines that are causing issues than the entire file, because if something in our assumptions change later that will make debugging more tricky, and plus I would consider it better practice.
There was a problem hiding this comment.
Looks good, but I think I would particularly like to see if @jakkdl and/or @TeamSpen210 have any comments as well, they've done a lot of work in this sort of area if I'm not mistaken.
There was a problem hiding this comment.
Yeah I, uh, think the vast majority of the formatting changes in this PR are negative wrt readability. I'm somewhat biased as I'm the author of the latest set of formatting improvements wrt type hints in Black, but the ruff authors have also intentionally skipped some of the (imo) improvements partly due to disagreeing with implications of style choices, and partly due to implementation details being difficult to replicate. Not finding the exact issue atm, but e.g. psf/black#4123 has very long and incredibly in-the-weed discussion about type hints. I also have some weak long-term worries about ruff gobbling up Black, but idk.
If black being a testing requirement is bad, then we can mark the tests with skipifs. Or if having the tests in themselves is bad, then remove them. test_run_black does seem somewhat overkill, but /shrug
The astor->ast changes seem good though.
Overall I'm not a hard no, but I vote against switching away from Black.
No that was the issue I was thinking about, it's linked at https://docs.astral.sh/ruff/formatter/black/#parenthesizing-long-nested-expressions |
|
Guess this ain't gonna happen. |
Removes a dependency, which is always good.
Apparently it's meant to be drop-in compatible, but clearly not.
Most of the test code relating to Black has been removed, because that entire area is an atrocity and needs to be vaporised.