Code formatting tools for Mailman project
Hey Everyone,
I am trying out some of the code formatting tools on Mailman repos to help reduce some efforts in development. The tools I am trying out is blue1 and isort2 to format code and imports. We've been using isort on some repos, but am planning to expand that to all projects.
I am also adding a new tox command tox -e format that should run the
appropriate commands locally for users to run.
The choice of tool, blue in this case, was mostly motivated by the design choices that align closest with Mailman's as Barry is a co-owner of the tool and authored Mailman's style guide as well.
If there are any issues with these tools, please let me know or feel free to open an issue on the project and ping me there.
I've started off with Postorius3, but will expand further to other projects. Core will probably be updated towards the last.
-- thanks, Abhilash Raj (maxking)
Abhilash Raj writes:
I am trying out some of the code formatting tools on Mailman repos to help reduce some efforts in development. The tools I am trying out is blue[1]
Please don't hurry to merge this to main/master. Remember that Python has been rejecting proposals to mass-PEP8 the stdlib for about 2 decades now.
I (speaking only for myself) have no objection going forward for new files and major refactorings. But if you do this all at once, I fear that most interesting diffs for the next year or so will be full of reformatting cruft, with a long tail of too-crufty-to-be-useful diffs on infrequently changed files for years. On the other hand, I don't find any Mailman code hard or annoying to read because of stylistic differences.
I'll take a look at the PRs but it will probably be a while (weeks) before I have a good sense for just how much disruption is involved.
and isort[2]
I'm cautiously in favor of this. Standardizing import format is a big enough improvement when reading code to justify the relatively small amount of churn.
I am also adding a new tox command
tox -e formatthat should run the appropriate commands locally for users to run.
What is the purpose of this? Anybody who runs it on a code base before the blue'd code gets merged is going to generate hard to read crufty PRs, no? I guess people with long-lasting feature branches may find it convenient in rebasing their branches (but I think that's going to be a massive PITA regardless[1]). I think the documentation should be *very* opinionated about getting input from core devs before blue'ing a file.
The choice of tool, blue
It's a good choice!
Footnotes: [1] Does anybody have experience in rebasing branches across a massive reformatting by upstream? Are there best practices here?
On 10/27/22 22:10, Stephen J. Turnbull wrote:
Abhilash Raj writes:
I am trying out some of the code formatting tools on Mailman repos to help reduce some efforts in development. The tools I am trying out is blue[1]
Please don't hurry to merge this to main/master. Remember that Python has been rejecting proposals to mass-PEP8 the stdlib for about 2 decades now.
So far, I did get it merged in Postorius, but that's the only one so far. Obviously git gives us powers to undo that, so we should still discuss this.
I (speaking only for myself) have no objection going forward for new files and major refactorings. But if you do this all at once, I fear that most interesting diffs for the next year or so will be full of reformatting cruft,
I am not sure I understand this part. Do you mean to say we'd be getting too may PRs with "formatting-only" changes?
Currently, my thought is to do a one-time format for the entire codebase and then add "blue --check" as a part of the "qa" CI gate. Then, for each subsequent MR that follows, people would need to run "tox -e format" before they commit and create MR. The hope there is also
On the other hand, I don't find any Mailman code hard or annoying to read because of stylistic differences.
That's true, the current state of code base isn't the motivation really for me personally :-)
My motivation here is to not have to push "oops, make flake8 gods happy" commits on top of my PRs and not have to worry about it when writing code.
While it isn't too huge of an issue, it is still manual effort that I feel can be automated to reduce some work and save time. My editor sometimes helps me with annoying red and yellow underlines, but I don't still see a need to have to resolve any formatting only error manually during development process.
From a contributor's perspective also, even though it is not a massive help, it reduces one less CI check (qa -- partially) you don't need to manually fix for.
I'll take a look at the PRs but it will probably be a while (weeks) before I have a good sense for just how much disruption is involved.
and isort[2]
I'm cautiously in favor of this. Standardizing import format is a big enough improvement when reading code to justify the relatively small amount of churn.
I am also adding a new tox command
tox -e formatthat should run the appropriate commands locally for users to run.What is the purpose of this? Anybody who runs it on a code base before the blue'd code gets merged is going to generate hard to read crufty PRs, no?
How so? If the existing code base is already auto-formatted, then the PRs would be just regular diffs, formatted with the same tool.
I am probably missing something here.
I guess people with long-lasting feature branches may find it convenient in rebasing their branches (but I think that's going to be a massive PITA regardless[1]). I think the documentation should be *very* opinionated about getting input from core devs before blue'ing a file.
Footnotes: [1] Does anybody have experience in rebasing branches across a massive reformatting by upstream? Are there best practices here?
Yeah, rebasing is going to be an issue, but I agree with you that it is an issue regardless.
I've tried to solve at least the "git-blame" issue by adding the rev of the commit with the "mass-refactor" into a file in the main repo that you can feed to "git blame --ignore-rev-file .git-blame-ignore-revs".
For editors, there is a way to set git config for the blame-ignore-file so it works across everything that uses git.
-- thanks, Abhilash Raj (maxking)
Abhilash Raj writes:
I (speaking only for myself) have no objection going forward for new files and major refactorings. But if you do this all at once, I fear that most interesting diffs for the next year or so will be full of reformatting cruft,
I am not sure I understand this part. Do you mean to say we'd be getting too may PRs with "formatting-only" changes?
No, I mean the diffs we do to localize errors. The day after you merge the reformatting PR, anybody who wants to identify a regression since Postorius 1.3.7 is going to get as many lines of " -> ' as they do of code changes. Half the time what appears to be a regression from future 1.3.8 to HEAD is going to turn out to be older than the reformat merge, same problem just less frequent. It gets less frequent over time, but it will take quite a while before the reformat merge consistently contributes less than 10% of changes, I expect.
Currently, my thought is to do a one-time format for the entire codebase and then add "blue --check" as a part of the "qa" CI gate. Then, for each subsequent MR that follows, people would need to run "tox -e format" before they commit and create MR. The hope there is also
Yes, this is a great idea for new files, or files you're refactoring to death anyway. But it has a potentially large downside of turning diffs that should be one-line changes into rafts of fixquote changes.
My motivation here is to not have to push "oops, make flake8 gods happy" commits on top of my PRs and not have to worry about it when writing code.
While it isn't too huge of an issue, it is still manual effort that I feel can be automated to reduce some work and save time.
Sure, but doing it globally also makes some work. The question is does it make more work (and annoyance) for everybody who does diffs than is experienced by the developer who needs to make a flake8 commit? I do a lot more diffs on Mailman than I do commits.
What is the purpose of this? Anybody who runs it on a code base before the blue'd code gets merged is going to generate hard to read crufty PRs, no?
How so? If the existing code base is already auto-formatted, then the PRs would be just regular diffs, formatted with the same tool.
Maybe tox -e format isn't a problem because you would add that tox target in the same commit (or later) you do the reformat, but if this is going to be policy, some people will aggressively reformat their code to that standard before the package is reformatted. Every once in a while you still see MRs in Python where somebody PEP8s a whole stdlib module where the contribution is a 2-line doc improvement. Those get rejected of course. We should do that too.
I've tried to solve at least the "git-blame" issue by adding the rev of the commit with the "mass-refactor" into a file in the main repo that you can feed to "git blame --ignore-rev-file .git-blame-ignore-revs".
For editors, there is a way to set git config for the blame-ignore-file so it works across everything that uses git.
That's very helpful for some operations. How to make it discoverable is something of a headache though -- I didn't know there was a blame-ignore-file!
Steve
On 10/28/22 00:04, Stephen J. Turnbull wrote:
Abhilash Raj writes:
I am not sure I understand this part. Do you mean to say we'd be getting too may PRs with "formatting-only" changes?
No, I mean the diffs we do to localize errors. The day after you merge the reformatting PR, anybody who wants to identify a regression since Postorius 1.3.7 is going to get as many lines of " -> ' as they do of code changes. Half the time what appears to be a regression from future 1.3.8 to HEAD is going to turn out to be older than the reformat merge, same problem just less frequent. It gets less frequent over time, but it will take quite a while before the reformat merge consistently contributes less than 10% of changes, I expect.
Ah, I get your point now. And, I agree with you that it would make it difficult to compare raw diffs.
Currently, my thought is to do a one-time format for the entire codebase and then add "blue --check" as a part of the "qa" CI gate. Then, for each subsequent MR that follows, people would need to run "tox -e format" before they commit and create MR. The hope there is also
Yes, this is a great idea for new files, or files you're refactoring to death anyway. But it has a potentially large downside of turning diffs that should be one-line changes into rafts of fixquote changes.
I don't see the tools actually supporting formatting only-diffs. It
_can_ be done by piping all the updated files to blue command, but
then reverting style changes in parts of file where there wasn't any
real change made would be more work than just fixing the flake8 errors
manually.
It sounds all-or-nothing kind of scenario when it comes to formatting tools, blue/black at least from what I can see.
My motivation here is to not have to push "oops, make flake8 gods happy" commits on top of my PRs and not have to worry about it when writing code.
While it isn't too huge of an issue, it is still manual effort that I feel can be automated to reduce some work and save time.
Sure, but doing it globally also makes some work. The question is does it make more work (and annoyance) for everybody who does diffs than is experienced by the developer who needs to make a flake8 commit? I do a lot more diffs on Mailman than I do commits.
I am trying to think if we can make that process better, after making a one-time code formatting change, through tooling or alternative commands to grok the diffs.
Are you usually looking for just anything that looks odd or typically any kind of difference that looks suspicious or just trying to learn the differences? Can just comparing the commits/commit messages? or Merge Requests merged since the feature branch would help since we mandate all changes go through MR workflow?
I don't know if I can speak much about comparing raw diffs, because I
very rarely end up doing that. I am usually looking at commit level data
and looking for MRs that last changed the point I am interested in with
git-blame for some contextual data around the MR. Merge commits have
the MR no to easily track where the change came from.
What is the purpose of this? Anybody who runs it on a code base before the blue'd code gets merged is going to generate hard to read crufty PRs, no?
How so? If the existing code base is already auto-formatted, then the PRs would be just regular diffs, formatted with the same tool.
Maybe tox -e format isn't a problem because you would add that tox target in the same commit (or later) you do the reformat, but if this is going to be policy, some people will aggressively reformat their code to that standard before the package is reformatted. Every once in a while you still see MRs in Python where somebody PEP8s a whole stdlib module where the contribution is a 2-line doc improvement. Those get rejected of course. We should do that too.
My intent with tox -e format was to aid with the formatting of the
changes folks would contribute, with the assumption that the entire
codebase is already formatted.
But yeah, we shouldn't really entertain format-only changes.
I've tried to solve at least the "git-blame" issue by adding the rev of the commit with the "mass-refactor" into a file in the main repo that you can feed to "git blame --ignore-rev-file .git-blame-ignore-revs".
For editors, there is a way to set git config for the blame-ignore-file so it works across everything that uses git.
That's very helpful for some operations. How to make it discoverable is something of a headache though -- I didn't know there was a blame-ignore-file!
Yeah, I agree. I just added to Postorius with the formatting changes, currently it doesn't exist for other repos since we haven't really done any of those formatting changes.
-- thanks, Abhilash Raj (maxking)
Abhilash Raj writes:
I don't see the tools actually supporting formatting only-diffs.
I don't know if git diff supports everything GNU diff does but --ignore-all-space would likely help quite a bit.
It shouldn't be that hard to write a blue-diff command for git:
- create a temporary branches at the relevant commits
- for each temporary branch 2.1 checkout the branch 2.2 run blue on the relevant .py files and commit
- diff the temparary branches and save the output
- delete the temporary branches
I don't know if this would help in practice, but the theory seems good.
Scripting that should be easy, and I can test it on the Postorius code. I'm not going to get to it before December, though, so if you want to do it first :-D
It _can_ be done by piping all the updated files to
bluecommand, but then reverting style changes in parts of file where there wasn't any real change made would be more work than just fixing the flake8 errors manually.
Is that necessary? Since the desired result is reformatted files without impairing the ability to diff, why would we revert the style changes?
I am trying to think if we can make that process better, after making a one-time code formatting change, through tooling or alternative commands to grok the diffs.
Additional tools to parse the diffs might be useful. Another possibility would be to diff the AST. :-O
Are you usually looking for just anything that looks odd or typically any kind of difference that looks suspicious or just trying to learn the differences?
Both of those are common. Typically it's something where I can't come up with a bisectable test, eg, a GUI regression. Then it's wait for rebuild, fire up the app, mess with the GUI, repeat ... so I don't actually bisect, once I have a bracket I just take that diff.
Can just comparing the commits/commit messages?
git diff --stat often helps isolate to a file.
or Merge Requests merged since the feature branch would help since we mandate all changes go through MR workflow?
I don't see how that helps if you're looking at a dozen commits touching that file on main.
I don't know if I can speak much about comparing raw diffs, because I very rarely end up doing that.
I try to avoid it, but about even in the large code bases I know best (XEmacs, Lisp and C) half the time I end up looking at diffs.
I am usually looking at commit level data and looking for MRs that last changed the point I am interested in with
git-blamefor some contextual data around the MR. Merge commits have the MR no to easily track where the change came from.
I rarely can pinpoint the relevant code without a diff, though. Most of the time the bad data is several frames up the stack, and was generated far from the visible error.
Also, you and I are likely to be working on code we just wrote or reviewed. We need to consider the drive-by contributor who is not going to know our code well.
Steve
participants (2)
-
Abhilash Raj -
Stephen J. Turnbull