Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added pre-commit hooks #421

Merged
merged 7 commits into from
May 20, 2022
Merged

Added pre-commit hooks #421

merged 7 commits into from
May 20, 2022

Conversation

Zabuzard
Copy link
Member

@Zabuzard Zabuzard commented Mar 29, 2022

Overview

This adds a pre-commit hook with an automatic installation to get rid of

Fuck you Spotless

commits 😆

Gradle installs the hook and the hook applies spotless and runs the unit tests.

Downside, commits now take about 10 seconds 2 seconds.

Tais993
Tais993 previously approved these changes Mar 29, 2022
Copy link
Member

@Tais993 Tais993 left a comment

Choose a reason for hiding this comment

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

I rather wait 10s than manually run spotless & commit & squash & force-push & hate Spotless :p

* gradle installs the hook
* hook executes spotless and unit tests
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Tais993
Tais993 previously approved these changes Mar 29, 2022
@borgrel
Copy link
Contributor

borgrel commented Mar 29, 2022

cant u get it to apply spotless with github actions?

@Zabuzard
Copy link
Member Author

Zabuzard commented Mar 29, 2022

@borgrel That would be pretty messy. Since then someone else creates new commits on "your branch", which interfers with your local branch. I.e. you have to do git pull each time after a git push then. And we pollute our git history then with "Spotless apply" extra commits after each commit. And if you let it modify the existing commit, then its even worse for the user, since their local branch is then incompatible with the remote branch.

Everyone and their mother uses pre-hooks for anything that actually changes code/commits for that reason.

The pre-commit hook is the standard solution for this and it works perfectly fine (once someone actually figures out how to configure it and set it up - i.e. what I had to do here 😆).

The fact that the change is applied to the commit before it is even created or pushed, simplifies things massively for everyone working with git. Git doesnt like it if u start manipulating commits, history starts getting incompatible then and the only solution out is force-push and throw-away-and-fresh-checkout.

@borgrel
Copy link
Contributor

borgrel commented Mar 29, 2022

nods

thanks for the explination

scripts/pre-commit Outdated Show resolved Hide resolved
scripts/pre-commit Outdated Show resolved Hide resolved
scripts/pre-commit Outdated Show resolved Hide resolved
Copy link
Contributor

@marko-radosavljevic marko-radosavljevic left a comment

Choose a reason for hiding this comment

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

Since we decided to go with a pre-commit hook, we don't need a previous spotlessApply on compile/build strategy.

That way, everyone can enjoy their own formatting, without getting annoyed with constant spotless re-formatting while they work.

If you agree, you can remove this line:

dependsOn 'spotlessApply'

@github-actions
Copy link

This pull request is stale because it has been open 30 days with no activity. Remove stale label, comment or add the valid label or this will be closed in 5 days.

@github-actions github-actions bot added the stale label Apr 30, 2022
@Zabuzard Zabuzard added valid This issue/PR is validated and ready to be picked. This auto adds items to TJ project board. and removed stale labels Apr 30, 2022
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Contributor

@marko-radosavljevic marko-radosavljevic left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for documenting it nicely. ☺️ ❤️

Consider this PR approved even if you force push some history cleanup.

@Zabuzard Zabuzard merged commit 032dee5 into develop May 20, 2022
@Zabuzard Zabuzard deleted the feature/pre_commit_hooks branch May 20, 2022 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority: normal valid This issue/PR is validated and ready to be picked. This auto adds items to TJ project board.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants