Format trailing-whitespaces and end-of-file with pre-commit #2204
Conversation
.pre-commit-config.yaml
Outdated
| @@ -39,5 +36,8 @@ repos: | |||
| - id: black | |||
| verbose: true | |||
|
|
|||
| # Vendored | |||
| exclude: ^com/win32comext/mapi/src/mapi_headers/.*$ | |||
There was a problem hiding this comment.
I've kept com/win32comext/mapi/src/mapi_headers ignored, but I really feel like when it comes to formatters (not linters/autofixers!), we could still apply them to vendored code. Especially given that it should be automatically formatted once https://pre-commit.ci/ is connected to this repo.
It's already the case with Black where it's run indiscriminately on the entire repo.
There was a problem hiding this comment.
In general I disagree - the "cost" of doing this is that it makes it more difficult to check the provenance of the file (ie, "is this the same file from SDK version a.b.c?" becomes far more difficult to answer). The benefits from auto-formatting (that when editing a file there's no ambiguity about what the new code should look like) doesn't apply as we don't expect the files to be edited - indeed, we expect git log for the file to list only the time it was vendored and any other changes are likely to be in error. For these MAPI files, I really can't see a single benefit to formatting them.
I can see how the case can be made to autoformat code we've "forked" though - there generally is an expectation of editing those files. Certainly if we had "vendored" python code I would have asked for black to exclude them. The closest we have to that in this repo are those "idle" files, which I consider forked.
There was a problem hiding this comment.
Relatedly, I consider Scintilla "vendored" too so only see downsides and no upsides to auto-formatting that.
There was a problem hiding this comment.
I understand your point, that is not something I had considered (or I had forgotten about), as well as the semantic differences between "vendored" and "forked".
There was a problem hiding this comment.
What about SWIG and AutoDuck?
Avasam
force-pushed
the
trailing-whitespaces-&-EOF
branch
from
9af87c9
to
0116357
Compare
Avasam
force-pushed
the
trailing-whitespaces-&-EOF
branch
from
0116357
to
b126854
Compare
Avasam
force-pushed
the
trailing-whitespaces-&-EOF
branch
from
4411d06
to
28708c0
Compare
Follow-up to #2034 for trailing whitespace changes. Merging #2222 would reduce changes here. You can hide whitespace changes to make the review easier.
These files would all be automatically formatted with pre-commit.ci.
Due to the nature of this PR containing a
.git-blame-ignore-revsentry, please do not squash-merge, I'll keep the history of this PR a clean 2 commits.@vernondcole the adodbapi changes should be quite easy to approve as it only contains trailing whitespace changes.