[Refactor] Apply clang-format to netaddress.h #22562
Conversation
This brings the entirety of netaddress.h into styling compliance with regard to the indentation of `public`/`private`/`protected` class blocks, per the styling guidelines.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Regarding DrathBot's conflict notices: most, if not all, of them can be resolved trivially as they are whitespace-only conflicts. |
Sorry, but this change seems like the worst of both worlds. You've opened a change to blanket fix code style, citing a low amount of activity, but at the same time, only fixed a portion of the style? I was going to suggest at least making the commit a scripted diff, so that it's more obviously correct, but that can't be done if this change is hand-crafted. |
|
From doc/developer-notes.md:
|
I was running into a similar issue recently. While this is probably a good rule for incorrect style in a single line, if a whole block is wrongly indented (e.g. 3 spaces vs 4 spaces), it makes editing that block harder because your editor might re-indent the whole block and you'd have to manually adjust the diff afterward to restore the incorrect indentation. See #22500 (comment) |
|
I don't object fixing the style. My eyes are hurt every time I encounter inconsistencies and it is indeed annoying when the editor automatically formats it in another way. OTOH it is in a clear violation of the guidelines, so maybe just remove that sentence from Ideally I would go for clang-format'ing all source files (scripted diff) at once and making sure that future violations paint the CI red. There is |
|
It seems there are many more valuable changes waiting for review, and review is a scarce resource. |
|
I can certainly change this to being done via a scripted-diff to include both the indentation and the pointer/reference alignment inconsistencies in one go for this specific file. I originally didn't include the pointer/reference alignment change here as that inconsistency is more wide-spread throughout the codebase (and not explicitly mentioned in There also seems to be a conflict between the
I felt that fixing the indentation inconsistency would improve the developer experience overall by providing consistency. I thought my explanation of the situation was through, and, at least for the indentation inconsistency, it is explicitly mentioned to be preferred in the @vasild in an ideal world, I agree that a single scripted-diff to clang-format the entire source tree would indeed be a "fix to fix all fixes" type solution, but past experiences lead me to believe that it would undoubtedly result in many many conflicts with other open PRs and only serve to drag out, complicate, or even indefinitely stall the review process. this is why I wanted to make this a narrow-focused PR on the single file that violates the indentation preference. |
I tried something like this in #6839, which was closed. I don't think anything has changed since then about re-formatting the whole code base. Also a red CI for "wrong" format is problematic by itself. See also my reply template: Thank you for your contribution. While this stylistic change makes sense on its own, it comes at a cost and risk for the project as a whole. The weak motivation for the change does not justify the burden that it places on the project. A burden could be any of the following:
For more information about refactoring changes and stylistic cleanup, see
Generally, if the style is not mentioned nor enforced by the developer notes, we leave it up to the original author to pick whatever fits them best personally and then leave it that way until the line is touched for other reasons. Let me know if you have any questions. |
|
IMO, please don't do this. Only make changes that you need to make, in files you need to change. Don't go randomly reformatting files. I'm pretty sure we explicitly discourage this. |
Ok. Might want to update the Pull Request template to reflect this sentiment then, as this was not a "random" reformat, but rather a targeted reformat that I felt fell within the guidelines specifically outlined within that document. Sorry for the inconvenience. |
Sounds good to me. I would refer to
Yes, sorry, "random" might be the wrong word. I just mean it comes out of the blue, it's not a section of the code you've been working on.
No problem, thanks for trying to contribute! |
7 participants
This brings the entirety of netaddress.h into styling compliance
with regard to the indentation of
public/private/protectedclass blocks, per the styling guidelines.
As outlined in the developer documentation, there should be
no indentation for
public/private/protectedclass blocks,and this file currently has a "mixed-usage" of indentation vs
non-indentation in this regard. This PR serves to bring the file
into uniform compliance with the standing guidelines, which
should prevent/ease further confusion.
This is a whitespace-only PR, as can be verified by enabling
the "Hide whitespace changes" option in GitHub's WebUI;
net 0/0 additions/deletions.
I know blanket refactors such as this are generally
discouraged, but seeing as there is little current activity with
this file at the moment, and that it is bringing it into styling
compliance, I feel it is appropriate/justified if for nothing else
than squashing a discrepancy in the existing code styling
that is in direct contradiction to the stated guidelines.
Note: I have specifically ignored the pointer/reference side portion
of clang-format here specifically to make this a whitespace-only
change-set. As such, anyone running the
clang-format-diff.pyscript manually against this commit will see further changes that
are intentionally omitted here, but could be added if deemed
appropriate.