Manually run fmt on all files under parquet#6328
Conversation
|
Thanks @etseidl The check certainly seems to find the problem: https://github.com/apache/arrow-rs/actions/runs/10619078161/job/29435831298?pr=6328 The only other thing I think is important here is to provide instructions (as comments) about how to fix the CI check if it fails. E.g. "if this test fails, run |
As a comment in |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Looks great to me -- thank you @etseidl
IF we get annoyed the formatting isn't working / CI fails too often we can figure out some way to get these files formatted directly via cargo fmt (e.g. change the macro incantantations etc)
| - name: Format arrow | ||
| run: cargo fmt --all -- --check | ||
| - name: Format parquet | ||
| # Many modules in parquet are skipped, so check parquet separately. If this check fails, run: |
There was a problem hiding this comment.
💯 thank you
The only other thing I think is important here is to provide instructions (as comments) about how to fix the CI check if it fails. E.g. "if this test fails, run
cargo fmt .....and check in the result" kindAs a comment in
rust.yml? Can do. I was also thinking some words in the formatting section ofCONTRIBUTING.mdwould be helpful.
Yes, the reason I think it is useful to include this information is so that if the CI test fails, the developer can just look at the CI failure report on github (rather than hunting around in the docs) and know how to fix it.
THis looks good to me -- thank you
|
🚀 |
|
Thanks again @etseidl -- sorry for the delay in merge |
2 participants
Which issue does this PR close?
Closes #6179.
Rationale for this change
Modules declared in a macro are skipped by rustfmt. This only appears to be an issue in the parquet crate.
What changes are included in this PR?
Runs rustfmt for all rust source files under parquet in the CI workflow. Also updates formatting instructions in
CONTRIBUTING.md.Are there any user-facing changes?
No