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

feat(git): enforce commit message validation #78

Merged
merged 8 commits into from
Feb 23, 2018

Conversation

apapko
Copy link
Collaborator

@apapko apapko commented Feb 7, 2018

Details

Enforce commit message validation to comply with conventional-commit. Fixes #55

Problem:
During squash and merge, git uses pull request title, ci-enforced to be formatted to comply with conventional-commit standard, as a merge commit message. These formatted commits are then picked up by our changelog library during the release to generate release entries.
Unfortunately, above flow doesn't work when pull request contains only one commit. In which case, git uses the commit message itself instead of the pull request title, which won't be picked up by the changelog library during release because it has not been formatted.

Solution:
In order to solve above issue and further improve commit message consistency, we will enable commit message validation. This will ensure that the commit messages are always formatted - whether you are doing squash and merge from the ui or manual merging, the validation will guarantee that the important commits will always make it to our release changelog.

Hopefully by now developers are used to simple and readable format and won't find enforced formatting too tedious. :

<type>(<optional scope>): <subject>

Please note, the scope is optional and if you are adding review comments or saving work in progress you can simply do:

wip: saving work
refactor: review comments

However, I would like to gather feedback from the team before enabling this feature.

Sample Output:

myshell$ git commit -m 'goose: bad message'
husky > npm run -s commitmsg (node v9.4.0)

INVALID COMMIT MSG: "goose" is not allowed type ! Valid types are: feat, fix, docs, style, refactor, perf, test, build, ci, chore, revert, proposal
goose: bad message
Please refer to commit message guideline:
https://github.com/salesforce/lwc/blob/master/CONTRIBUTING.md#commit

Does this PR introduce a breaking change?

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:
Please check if your PR fulfills the following requirements:

@apapko apapko changed the title feat(git): enforce commit message validation to comply with conventional-commits feat(git): enforce commit message validation Feb 7, 2018
@salesforce-best-lwc-internal
Copy link

Benchmark comparison

Base commit: 0ef3733 | Target commit: a328f71

benchmark base(0ef3733) target(a328f71) trend
append-1k.benchmark:benchmark-table-component/append/1k 343.69 (± 7.97 ms) 335.67 (± 4.06 ms) 👍
clear-1k.benchmark:benchmark-table/clear/1k 38.83 (± 0.91 ms) 39.47 (± 1.27 ms) 👌
create-10k.benchmark:benchmark-table-component/create/10k 2187.80 (± 18.93 ms) 2225.12 (± 32.51 ms) 👎
create-1k.benchmark:benchmark-table-component/create/1k 251.00 (± 3.43 ms) 255.59 (± 6.18 ms) 👎
update-10th-1k.benchmark:benchmark-table-component/update-10th/1k 116.44 (± 1.49 ms) 119.64 (± 2.59 ms) 👎

"multiple":false
},
"warnOnFail":false,
"maxSubjectLength":100,
Copy link
Contributor

Choose a reason for hiding this comment

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

140 tweet anyone? 🐦 kidding alone, what's the one line limit in GitHub before ... gets generated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can tweak it to our likings, but quick search revealed this 50/72 rule, where ideal length is 50 characters - but in my opinion its a bit short given that we have to include type and scope. https://medium.com/@preslavrachev/what-s-with-the-50-72-rule-8a906f61f09c

@salesforce-best-lwc-internal
Copy link

Benchmark comparison

Base commit: 66454b4 | Target commit: 3b18319

benchmark base(66454b4) target(3b18319) trend

@diervo
Copy link
Contributor

diervo commented Feb 8, 2018

@apapko rebase and push

@salesforce-best-lwc-internal
Copy link

Benchmark comparison

Base commit: 9d9d7d9 | Target commit: ca2d530

benchmark base(9d9d7d9) target(ca2d530) trend

@caridy
Copy link
Contributor

caridy commented Feb 8, 2018

this PR has a lot of commits, and changes that were already integrated into master!

@apapko apapko changed the base branch from master to apapko/commit-validation-base February 8, 2018 18:59
@apapko apapko changed the base branch from apapko/commit-validation-base to master February 8, 2018 19:00
@apapko
Copy link
Collaborator Author

apapko commented Feb 8, 2018

@caridy it seem like there was a git glitch, it didn't pick up my rebase changes. @trevor-bliss showed me a trick to switch base and it solve the issue.

Copy link
Contributor

@trevor-bliss trevor-bliss left a comment

Choose a reason for hiding this comment

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

BTW I got the following error when I tried to commit locally:

.git/hooks/pre-commit: line 2: ./node_modules/pre-commit/hook: No such file or directory

I fixed by running rm .git/hooks/pre-commit

@apapko apapko merged commit 745e44e into master Feb 23, 2018
@apapko apapko deleted the apapko/commit-validation branch February 28, 2018 21:30
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.

I can still commit a wrong title (non-conventional)
7 participants