Feature/pre commit

[maltevesper]

trafficstars

PR Info

• Closes SeaQL/sea-orm/issues/591

• Dependencies:
  • SeaQL/sea-orm/pull/611

• Dependents:

Adds

• Add pre-commit hooks to format and lint

Fixes

• [ ]

Breaking Changes

• [ ]

Changes

• [ ]

[maltevesper]

Unfortunately I have not seen #611 before I created it (it was not linked in the issue). So now the project has to decide between pre-commit and vanilla bash/batch scripts.

In my opinion the main upside of using pre-commit over vanilla scripts is that it handles some corner cases, i.e. I have not staged everything, in that case one would expect the tools to only run on what is to be committed, and not the unstaged files. pre-commit handles this automatically, while vanilla hook scripts have to take additional steps to ensure the correct behaviour.

[maltevesper]

By the way I think I should add something along these lines to the documentation, I am not sure where, and it seems you have more thoughts on what should be done with this signoff discussion over in #611):

Be sure to install pre-commit for this repository by installing pre-comit on your system and enabling it for the repository:

pip install pre-commit       # install pre-commit on system
pre-commit install             # install pre-commit hooks for repo (run inside the repo folder)

[maltevesper]

Not changing https://github.com/SeaQL/sea-orm/blob/9c4cdd84a5a70def5e0ca375728ff631132eebbf/.github/workflows/rust.yml#L108 to workspace is not an oversight, while the docs state that all is an deprecated alias for workspace, cargo does not like workspace with fmt

[maltevesper]

Also is it intentional that the cargo pipeline does NOT run cargo clippy -- -D warnings (note the last part) I would expect that this way clippy is run but the output would be ignored?

[billy1624]

Hey @maltevesper, thank you so much for the contributions! I think we better perform the clippy checking during CI checks, instead of putting the burden on developer's machine. We have tried https://github.com/marketplace/actions/rust-clippy-check before but it turns out making the "file changes" tab in PR very messy and hard to review the changes. So, currently we will run clippy check and cargo fmt regularly before every release.

[maltevesper]

Hey @billy1624, the wuestion is how would you suggest to proceed, issue #591 still stands as it is. IMHO having a precommit hook can be a big help, especially if several commits are done for a feature before it is fed to the pipeline. In my experience this leads to fixup commits at the end which are a bit out of place:

commit 1 / new feature commit 2 / test for feature ... commit 3 / fix formater issues commit 4 / fix linter warnings

a pre commit script can help to reduce these low value commits (which only fix linter,formater thing). I know that there is interactive rebase & squasing. But not everyone does, and if a feature is implemented across multiple commits, this also tends to dirty the commit history, since it is hard to fixup linter bits on the right commits after the fact.

While I have amde negative experiences with this and use precommit as a tool to help reduce this, it might very well be that this is no problem in practice for sea-orm. Also, pre-commit can not replace the pipeline checks, since it depends on the user actually installing the hooks and not surpessing them with --no-verify.

That said, do you actually want to implement pre-commit hooks? If yes what should they do, so that I can adjust this PR? If no, I suggest you reject this PR and close the issue as wont fix. No hard feelings. I might potentially pick up another issue then.

One last note regarding running clippy on the dev machine: I figured every developer surely (should) compile check his changes, so most things should be cached by rust and the checks should be relativly fast anyways. But I might be wrong there, I am only now moving to normal sized projects with rust, and had only toy projects before.

[billy1624]

Hey @maltevesper, appreciate your thoughts and efforts! I think we can incorporate precommit hook in SeaORM to perform clippy and cargo-fmt checks but there are no guarantee developers would run it on their local machine. That's what holding me back.

[billy1624]

Thoughts? @tyt2y3

[maltevesper]

Hi, I am on vacation but will have a look in a week. What should I do here? Last I checked there where still second thoughts.

I can resolve the mergeconflicts.

Regarding the second thoughts about pre commit hook vs pipeline: I think the pipeline should check everything, since contributers might not run precommit hooks. The job of the pre commit hook is to ensure people that use them that they dont have to create fixup commits.

That said, is there anything else you want the hook doing? Or is it a simple resolve mergeconflicts?

Final note git commit --no-check skips running (precommit) hooks in case you dont want to wait.

[maltevesper]

@billy1624 I attempted a rebase, I am a little bit surprised to run into:

Warning: can't set format_code_in_doc_comments = true, unstable features are only available in nightly channel.

Apparenlty you set that option 11 month ago, and I assume I am doing something wrong now when running rust-fmt. Can you enlighten me? (I assume running this with a nightly rust is not the right solution)

[tyt2y3]

I don't think that's an error, that's only a warning, so you are good. As a matter of fact, the nightly fmt differs the stable fmt so what I do these days is:

cargo +nightly fmt
cargo fmt

The reason nightly was introduced in the first place is we wanted to format the code inside our doc tests.

[billy1624]

Hey @maltevesper, sorry for the conflict. I think similar code has been merged

• https://github.com/SeaQL/sea-orm/pull/840
• https://github.com/SeaQL/sea-orm/pull/919

Please check if you have anything to add.

[tyt2y3]

Yeah the CI step works fine so far. Thanks