Skip to content

ARROW-10952: [Rust] Add pre-commit hook#8953

Closed
mqy wants to merge 4 commits intoapache:masterfrom
mqy:ARROW-10952_Add-pre-commit-hook
Closed

ARROW-10952: [Rust] Add pre-commit hook#8953
mqy wants to merge 4 commits intoapache:masterfrom
mqy:ARROW-10952_Add-pre-commit-hook

Conversation

@mqy
Copy link
Copy Markdown
Contributor

@mqy mqy commented Dec 17, 2020

If a commit contains only fixes for rust format, it would be very sad to wait for the slow CI checking done, and it may block the CI checking for other PRs.

So, this PR adds a git pre-commit hook file, which performs the "cargo fmt --check"

@codecov-io
Copy link
Copy Markdown

codecov-io commented Dec 17, 2020

Codecov Report

Merging #8953 (32177e9) into master (d65ba4e) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #8953   +/-   ##
=======================================
  Coverage   83.25%   83.25%           
=======================================
  Files         196      196           
  Lines       48116    48192   +76     
=======================================
+ Hits        40059    40124   +65     
- Misses       8057     8068   +11     
Impacted Files Coverage Δ
rust/parquet/src/arrow/array_reader.rs 77.00% <0.00%> (-0.56%) ⬇️
rust/parquet/src/arrow/schema.rs 91.31% <0.00%> (-0.50%) ⬇️
rust/parquet/src/encodings/encoding.rs 95.24% <0.00%> (-0.20%) ⬇️
rust/parquet/src/file/statistics.rs 93.80% <0.00%> (ø)
rust/arrow/src/array/array_binary.rs 90.73% <0.00%> (+0.21%) ⬆️
rust/parquet/src/schema/types.rs 90.19% <0.00%> (+0.26%) ⬆️
rust/datafusion/src/datasource/parquet.rs 95.62% <0.00%> (+0.30%) ⬆️
rust/parquet/src/arrow/arrow_reader.rs 91.25% <0.00%> (+0.66%) ⬆️
rust/parquet/src/file/metadata.rs 91.82% <0.00%> (+0.77%) ⬆️
rust/datafusion/src/physical_plan/parquet.rs 79.56% <0.00%> (+0.91%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d65ba4e...32177e9. Read the comment docs.

@github-actions
Copy link
Copy Markdown

Copy link
Copy Markdown
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this out locally and it worked well for me (MacOS). I typically don't use git commit hooks, but I think this is a nice contribution to assist those who would like to do so.

what do you think @nevi-me / @jorgecarleitao / @andygrove / @Dandandan ?

Comment thread rust/pre-commit.sh Outdated
@mqy mqy requested a review from alamb December 18, 2020 03:52
@Dandandan
Copy link
Copy Markdown
Contributor

Pre-commit hooks 👍

First thoughts:

  • Why not just fmt instead of check?
  • Would it make sense to run clippy too?

Copy link
Copy Markdown
Member

@jorgecarleitao jorgecarleitao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I haven't used git hooks much for Rust, but I think it is a good idea. :) Thanks @mqy for this.

Comment thread rust/README.md Outdated
```

We can use "git pre-commit hook" to automate this process: copy or soft link [pre-commit.sh](pre-commit.sh)
as file `.git/hooks/pre-commit`.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make it dead simple to people less experienced in git, would it be possible to paste the command here? E.g.

cp rust/pre-commit.sh ../.git/hooks/pre-commit

or

ln ...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorgecarleitao good idea!

I have tried this way but finally aborted.
One of the reason is: one line command for copy/paste is not enough -- if the pre-commit file already exists, copy/link may fail or override them unexpectedly, thus additional check/prompt would make the commands lengthy.

Anyway, let me try again.

@mqy
Copy link
Copy Markdown
Contributor Author

mqy commented Dec 18, 2020

Pre-commit hooks 👍

First thoughts:

  • Why not just fmt instead of check?
  • Would it make sense to run clippy too?

I'm glad to add above tasks as our new client side tiny git flow :) Let me try!

@mqy
Copy link
Copy Markdown
Contributor Author

mqy commented Dec 19, 2020

Updated. NOTE: /bin/sh was changed to /bin/bash

Although I had a few experience on TTY color before, honestly speaking, I'm not quite confident about the color behavior on various shells. Feed back welcome!

@alamb
Copy link
Copy Markdown
Contributor

alamb commented Dec 19, 2020

I think this is looking good @mqy -- thanks for the doc updates. Given it is an opt in and well documented, I am merging this in and we can iterate in subsequent PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants