chore: Update clang-format and prettier with pre-commit#5709
chore: Update clang-format and prettier with pre-commit#5709
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5709 +/- ##
=========================================
- Coverage 78.8% 78.7% -0.1%
=========================================
Files 814 814
Lines 71305 71639 +334
Branches 8351 8446 +95
=========================================
+ Hits 56196 56354 +158
- Misses 15109 15285 +176 see 202 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| # Then, run the following command to install the git hook scripts: | ||
| # - `pre-commit install` | ||
| # You can run all configured hooks against all files with: | ||
| # - `pre-commit run --all-files` |
There was a problem hiding this comment.
I prefer this command:
pre-commit run --from-ref develop --to-ref HEAD
--all-files usually reformats the external files, which we don't want
There was a problem hiding this comment.
Update: I see you fixed that, it'll still be much more efficient
There was a problem hiding this comment.
Can we update the comment in this file to match the above command? Usually the --all-files takes much longer and isn't necessary.
There was a problem hiding this comment.
The comments say:
# You can run all configured hooks against all files with:
# - `pre-commit run --all-files`
# To manually run a specific hook, use:
# - `pre-commit run <hook_id> --all-files`
# To run the hooks against only the files changed in the current commit, use:
# - `pre-commit run`
I don't see what's wrong - it specifically has a comment about the command that does not include --all-files.
There was a problem hiding this comment.
If you're referring to the --from-ref and --to-ref flags, I added commits to the PR containing them and they failed with fatal: ambiguous argument 'develop..HEAD': unknown revision or path not in the working tree., so I don't want to include a command that may or may not work.
There was a problem hiding this comment.
I assumed that was some CI issue rather than a problem with the CLI. But if you're not sure, we can leave it out.
|
I am little worried about missing the commit which changes The problem with updating this file is that it requires non-linear history if anything else is being changed in the same PR. So in order to do it we should either allow non-linear history, or move only formatting changes from this PR (i.e. no workflow or formatting configuration changes) to a different PR, and then add its merge commit (and that of #5657 ) to |
Yes, I can move the formatting changes to a separate PR, although we will have to ignore the clang-formatting error it will generate as .clang-format currently doesn't support Proto files and will complain loudly if it sees one. |
This could probably also be resolved by manually pushing 2 commits from one PR (one that actually does the changes, one that changes the configuration). |
I already created #5711. But it does have some clang-format config changes to make it happy. |
|
I am happy to see the progress made here, esp with #5711 and XRPLF/ci#46 |
|
Updates:
|
If it's minor then I think that's fine to keep in this PR. I only wanted the separate PR #5711 because the formatting literally rewrote all |
|
While we're at it, we may want to consider adding some of these to the list: https://github.com/pre-commit/pre-commit-hooks |
@mvadari this is being prepared now: |
I would also recommend |
|
I was also looking at this: https://github.com/streetsidesoftware/cspell-cli Which would cut down on the number of random PRs that just fix random typos |
@bthomee I tested that this will work ... however I do not see how that is useful given the rewrite in |
I would wait with this for next occasion, since I guess there would be fair amount of experimentation and churn involved. Given that there are likely to be many typoes in the codebase. 🙃 |
Yeah, that's fair. Do you mind if I experiment with this in a separate draft PR? |
Prettier already rewrites JSON files. I modified some of the strategy matrix JSON files and then tried to get clang-format to do anything, but it wouldn't fix them, which I found odd since I earlier had to add: to get it to stop complain about seeing a file in a format it didn't support. But since adding |
3 participants
High Level Overview of Change
The change updates how
clang-formatis called in CI and locally, and addsprettierto thepre-commithook. Proto files are now also formatted, while external files are excluded.Context of Change
This PR was inspired by #5665, which enabled
prettieras a pre-commit hook, and updated theclang-formatto move some common stanzas out of language-specific sections. The current changes do the same, but removes Javascript as a language (since we have no such files) and Json (since Prettier formats them), while adding Proto (since we do have such files).Finally, there is no hosted pre-commit hook for
prettieranymore, so installing it locally is therefore the most straightforward option. For consistency, this PR also makes the change toclang-format. This PR provides instructions on how to install the hooks locally, including to ensure their versions match what is actually used in CI.Type of Change
.gitignore, formatting, dropping support for older tooling)