scripted-diff: Sort test includes #18673
Conversation
-BEGIN VERIFY SCRIPT- # Mark all lines with #includes sed -i --regexp-extended -e 's/(#include <.*>)/\1 /g' $(git grep -l '#include' ./src/bench/ ./src/test ./src/wallet/test/) # Sort all marked lines git diff -U0 | ./contrib/devtools/clang-format-diff.py -p1 -i -v -END VERIFY SCRIPT-
-BEGIN VERIFY SCRIPT- ./contrib/devtools/copyright_header.py update ./ -END VERIFY SCRIPT-
maflcko
force-pushed
the
2004-testSortIncludes
branch
from
fa3a43c to
fa488f1
Compare
|
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. |
|
ACK just for the scripted-diff 👏
or not stage in the first place? |
|
I think generally accepted good practice is to go from local to global for includes (ie file's header, then local project imports, then library imports, then std imports). See https://stackoverflow.com/questions/2762568/c-c-include-header-file-order. That seems to be the de facto standard across much of this repo. Is there any way you can modify your scripted diff to preserve this ordering? |
|
@jnewbery I went ahead and manually fixed up the 7 files where the boost/stdlib includes were not last. Those files had this issue as a pre-existing condition and this is not something that can be solved by |
| #include <clientversion.h> | ||
| #include <hash.h> // For Hash() | ||
| #include <key.h> // For CKey | ||
| #include <key.h> // For CKey |
There was a problem hiding this comment.
No, it is part of the scripted-diff, which you can verify by running ./test/lint/commit-script-check
There was a problem hiding this comment.
Thanks -- I need to upgrade my scripted-diff-fu
|
ACK fa4632c A |
|
Very rough code review ACK. Thanks Marco! |
6 participants
When writing tests, often includes need to be added or removed. Currently the list of includes is not sorted, so developers that write tests and have
clang-formatinstalled will either have an unrelated change (sorting) included in their commit or they will have to manually undo the sort.This pull preempts both issues by just sorting all includes in one commit.
Please be aware that this is NOT a change to policy to enforce clang-format or any other developer guideline or process. Developers are free to use whatever tool they want, see also #18651.
Edit: Also includes a commit to bump the copyright headers, so that the touched files don't need to be touched again for that.