-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
core: add cz-customizable to establish a commit message convention #3499
Conversation
"path": "./node_modules/cz-customizable" | ||
}, | ||
"cz-customizable": { | ||
"config": "./.cz-config.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think you forgot this file :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (final nits)
.cz-config.js
Outdated
// report: Report, UI, renderers | ||
// cli: CLI stuff | ||
// extension: Chrome extension stuff | ||
// misc: Something else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line
@@ -0,0 +1,29 @@ | |||
'use strict'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
license header since it's js
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
@googlebot u high, bro? |
CLAs look good, thanks! |
{value: 'core', name: 'core: Driver, gather, (non-new) audits, LHR JSON, etc'}, | ||
{value: 'tests', name: 'tests: Tests, smokehouse, etc'}, | ||
{value: 'docs', name: 'docs: Documentation'}, | ||
{value: 'deps', name: 'deps: Dependency bumps only'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about this one, there are frequently dependency bumps included with other changes, do we want to enforce a separate PR for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clarity on enforcement seems good to iron out re: separate PRs or distinct commits but shouldn't block this IMO
lgtm
To use:
yarn global add commitizen
git add ....
git cz
Only necessary for first commit in a branch, to set the PR commit name. (Since we squash) followup commits can be whatever.
Later we can look at getting https://github.com/marionebl/commitlint running in a github commit status bot to check our PR titles.