-
Notifications
You must be signed in to change notification settings - Fork 810
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
yapf: use yapf instead of autopep8 #10860
Conversation
4712a7f
to
7d25d4a
Compare
7d25d4a
to
73faad1
Compare
6cf48e6
to
9e63af6
Compare
.style.yapf
Outdated
coalesce_brackets=False | ||
|
||
# The column limit. | ||
column_limit=99 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming this is limit of line length, our autopep8 setting was 132. I hope using 132
will reduce style change made by this migration.
trafficserver/tools/autopep8.sh
Line 91 in 40961fb
--max-line-length 132 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially tried 132 locally, but I think the diff is actually larger if we set the line length to that. In reality, if you look at our .py tests before this patch, the community has kind of implicitly respected about a 80-100 character limit while autpep8 didn't "expand" things to 132 characters. yapf will actively expand function parameters and arguments and the like to 132 if we set that size. I'm not opposed to setting 132, I'm just stating it didn't seem like using 132 reduced the patch size but rather increased it. I can switch things to 132 if we like.
The reason the patch is so large is because of the three big Python formatters (yapf, autopep8, black), autopep8 is the least "opinionated". yapf enforces much more style over autopep8. It enforces things at the level more akin to clang-format, for instance. I'm not sure that's bad, but it is how it is.
I guess the big question is whether we think the changes are a net benefit or not. I feel like we kind of need to move off of autopep8 because I've lost confidence in the project - 6 weeks after an issue was filed about Python 3.12 and there's not even so much as a comment on the issue from any maintainers. Not even when I explicitly (and politely) asked for status with an @ mention of the repo author have they replied after 8 days.
I intend, if I have time, to create a separate patch seeing how things look with the black formatter so we can compare. yapf has a pretty strong community which is a big plus, so I'll first investigate who and how much black is supported before trying them. Like all formatters, the goal is to find something "acceptable". "Beyond awesome" is probably not attainable.
Anyway, thanks for looking at the patch. Feel free to provide other thoughts. Or even to try things out locally, if you like. It's pretty easy to experiment by using my first to commits in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, let's go with 99
, if we want, we can do it later. I agree with we should move away from autopep8 now. Also, yapf
might not be the best formatter, but as far as it works like clang-format, it's acceptable.
Thanks for working on this 👍 Please resolve conflicts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed things to a 132 line limit and it looks better than I remembered. I think things look OK.
9e63af6
to
3bb74a1
Compare
autopep8 does not support Python 3.12, which is a requirement for moving to fedora:39. The following ticket has been open without comment from the development community for 6 weeks: hhatto/autopep8#712 Transitioning to yapf is easy, and the community behind it seems stronger. I think moving forward it is a better Python formatter for us.
3bb74a1
to
3f754ec
Compare
autopep8 does not support Python 3.12, which is a requirement for moving to fedora:39. The following ticket has been open without comment from the development community for 6 weeks: hhatto/autopep8#712 Transitioning to yapf is easy, and the community behind it seems stronger. I think moving forward it is a better Python formatter for us.
autopep8 does not support Python 3.12, which is a requirement for moving
to fedora:39. The following ticket has been open without comment from
the development community for 6 weeks:
hhatto/autopep8#712
Transitioning to yapf is easy, and the community behind it seems
stronger. I think moving forward it is a better Python formatter for us.
For Review
Note that this review is split up into three commits: