Add clang-format to the pre-commit. #222
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Before merging, we'll want to check that the relevant section in the developer documentation instructions are clear and accurate for future devs setting up |
|
Also: way too many commits in this branch...? Need another rebase? |
4 tasks
It's going to be because Edit: it looks a lot better now... commit-wise at least. |
Requires tdms/build/compile_commands.json to exist though :(
* First pass run clang-format over the whole codebase. * Fix TODO about namespace indentation. * Tell clang-format to ignore .m files (MATLAB). * Potentially controversial. Line length to 80 characters. * Turn off cppcheck and cpplint (for now). Remove clang-tidy. I think we only want the static linters that don't need build output. That way linting can be a separate (fast) job that can run in pre-commit. We might consider also binning cppcheck if it's too slow. * Confirming that clang-format respects doxygen format comments. It does.
willGraham01
force-pushed
the
wgraham-cpp-linter
branch
from
29c6c1b to
609fde1
Compare
5 tasks
|
I would advocate changing this PR's title and making it "Ready for review" as-is. Need to tweak the dev docs? Or are you happy? |
I don't think the dev docs need to change, at least for this one. Marked it as ready for review, and updated the main body of the PR to reflect the fact that it doesn't require Feel free to hit the merge button when your review goes in 👍 |
samcunliffe
changed the title
samcunliffe
approved these changes
Feb 20, 2023
4 tasks
willGraham01
added a commit
that referenced
this pull request
Feb 23, 2023
* Cherry-pick Sam's linting * Add flag to Source class to indicate empty array * Add flag to Source class to indicate empty array * Indentation * Initialise values rather than assign * Refactored source updates - flagged mystery indexing * Pre-commit consistency with #223 * Update clang-format to #222 settings * Fix out-of-order initilisation warning * Revert pre-commit to suppress GitHub failures until #223 is ready * Remove unused variables in updating Ksource terms * Start refactoring the refactored functions. Flag more inconsistencies * Missing std:: * Merge E_s steady-state updates into one function (revert files to previous commit if undesired) * Fix bugs in unification (IE Will can't type) * Pull out E_s source updates, and wrap to avoid empty-source updates * Pull out H-source updates, don't add empty array checks yet * Fully refactor H-field, now to find the bug... * Add that newline to make pre-commit happy * Missed a -1 in ONE place... does this fix the bug? * Remove useless comment * Apply Peter's suggestion on #226 * Add Peter's resolution to #225 * Apply Sam's suggestions for SourceIndex struct * Small doc changes that didn't get caught
willGraham01
added a commit
that referenced
this pull request
Mar 6, 2023
* Cherry-pick Sam's linting * Add flag to Source class to indicate empty array * Add flag to Source class to indicate empty array * Indentation * Initialise values rather than assign * Refactored source updates - flagged mystery indexing * Pre-commit consistency with #223 * Update clang-format to #222 settings * Fix out-of-order initilisation warning * Revert pre-commit to suppress GitHub failures until #223 is ready * Remove unused variables in updating Ksource terms * Start refactoring the refactored functions. Flag more inconsistencies * Missing std:: * Merge E_s steady-state updates into one function (revert files to previous commit if undesired) * Fix bugs in unification (IE Will can't type) * Pull out E_s source updates, and wrap to avoid empty-source updates * Pull out H-source updates, don't add empty array checks yet * Fully refactor H-field, now to find the bug... * Add that newline to make pre-commit happy * Missed a -1 in ONE place... does this fix the bug? * Remove useless comment * Apply Peter's suggestion on #226 * Add Peter's resolution to #225 * Apply Sam's suggestions for SourceIndex struct * Small doc changes that didn't get caught
willGraham01
added a commit
that referenced
this pull request
Mar 7, 2023
* Cherry-pick Sam's linting * Add flag to Source class to indicate empty array * Add flag to Source class to indicate empty array * Indentation * Initialise values rather than assign * Refactored source updates - flagged mystery indexing * Pre-commit consistency with #223 * Update clang-format to #222 settings * Fix out-of-order initilisation warning * Revert pre-commit to suppress GitHub failures until #223 is ready * Remove unused variables in updating Ksource terms * Start refactoring the refactored functions. Flag more inconsistencies * Missing std:: * Merge E_s steady-state updates into one function (revert files to previous commit if undesired) * Fix bugs in unification (IE Will can't type) * Pull out E_s source updates, and wrap to avoid empty-source updates * Pull out H-source updates, don't add empty array checks yet * Fully refactor H-field, now to find the bug... * Add that newline to make pre-commit happy * Missed a -1 in ONE place... does this fix the bug? * Remove useless comment * Apply Peter's suggestion on #226 * Add Peter's resolution to #225 * Apply Sam's suggestions for SourceIndex struct * Small doc changes that didn't get caught
willGraham01
added a commit
that referenced
this pull request
Mar 29, 2023
* Cherry-pick Sam's linting * Add flag to Source class to indicate empty array * Add flag to Source class to indicate empty array * Indentation * Initialise values rather than assign * Refactored source updates - flagged mystery indexing * Pre-commit consistency with #223 * Update clang-format to #222 settings * Fix out-of-order initilisation warning * Revert pre-commit to suppress GitHub failures until #223 is ready * Remove unused variables in updating Ksource terms * Start refactoring the refactored functions. Flag more inconsistencies * Missing std:: * Merge E_s steady-state updates into one function (revert files to previous commit if undesired) * Fix bugs in unification (IE Will can't type) * Pull out E_s source updates, and wrap to avoid empty-source updates * Pull out H-source updates, don't add empty array checks yet * Fully refactor H-field, now to find the bug... * Add that newline to make pre-commit happy * Missed a -1 in ONE place... does this fix the bug? * Remove useless comment * Apply Peter's suggestion on #226 * Add Peter's resolution to #225 * Apply Sam's suggestions for SourceIndex struct * Small doc changes that didn't get caught
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🥇 Here's every single warning: pre-commit-on-all.txt
linting.yamlWorkflow renamed to "Run code linting" (previously "Run python code linting").
.pre-commit-config.yamlInvokes pocc/pre-commit-hooks to force
C++linting:clang-formatis pointed at our.clang-formatFuture considerations:
We looked at a bunch of other pre-commit tools. Here's some stuff about them that we ended up not including in this PR but might be wanted going forward.
clang-tidycan be pointed to our compile commands. This requirescompile_commands.jsonto be present. Can be done by passingDCMAKE_EXPORT_COMPILE_COMMANDS=ONwhen invokingcmake.cpplintfilterswhitespace/commentsdue to a conflict withDoxygenandclang-tidystandards. Suppresseslegal/copyrightbecause our developer docs don't require a copyright notice in header files. Runs quietly so we don't know its there if it has nothing to say.