-
Notifications
You must be signed in to change notification settings - Fork 61
Improvement: indent ignore list and unification of indent scripts [1/2] #1688
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
base: main
Are you sure you want to change the base?
Conversation
|
I just noticed that t8indent.sh and check_if_file_indented.sh are nearly identical and i either have to replicate code and put it into check_if_file_indented.sh or unify those files. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1688 +/- ##
=======================================
Coverage 74.02% 74.02%
=======================================
Files 98 98
Lines 18577 18577
=======================================
Hits 13751 13751
Misses 4826 4826 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Unified both scripts. However, there is still a bug since now all files are reported as indented by our pre-commit hook. |
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.
Nice changes! Just small comments
|
|
||
|
|
||
| # Iterate over all arguments and throw | ||
| # aways those filenames that we should ignore. |
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.
| # aways those filenames that we should ignore. | |
| # away those filenames that we should ignore. |
| WANTSOUT=1 | ||
| fi | ||
| done | ||
| if [ -z "$WANTSOUT" ]; then |
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.
A few more comments what is happening here for which options would be nice
| # files instead of printing the changes to stdout. The --style=file | ||
| # arguments tells clang-format to look for a *.clang-format file. | ||
| FORMAT_OPTIONS="-i --style=file" | ||
| # |
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.
Maybe also state here that you can use the option -o ?
| echo "Please install clang-format version $REQUIRED_VERSION_STRING" | ||
| exit 1 | ||
| fi | ||
| usage="$0 [FILE_TO_INDENT]" |
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.
Maybe add USAGE: ?
Describe your changes here:
A slight overall of our indent scripts.
Since clang-format only supports ignore files from version 18 and we use version 17, some bash magic was required to manually parse the list of ignored files.
We should consider updating to clang-format >= 18.
I cleaned this up by unifying the files such that "t8indent" is the only file with indentation code and has a new option "NO_CHANGE" and "check_if_file_indented" calls "t8indent" with that option.
Edit: I also fixed an unrelated typo in a comment in a t8_geometry file which kept the CI pipeline from failing. Usually i should have opened a new PR, but this was literally just one character and it was in a comment. So i thought i would get away with sneaking it into this PR to make the pipeline succeed.
Closes #1687
Preparation for #1675 and should be merged before #1522
All these boxes must be checked by the AUTHOR before requesting review:
Documentation:,Bugfix:,Feature:,Improvement:orOther:.All these boxes must be checked by the REVIEWERS before merging the pull request:
As a reviewer please read through all the code lines and make sure that the code is fully understood, bug free, well-documented and well-structured.
General
Tests
If the Pull request introduces code that is not covered by the github action (for example coupling with a new library):
Scripts and Wiki
script/find_all_source_files.scpto check the indentation of these files.License
doc/(or already has one).