Skip to content
This repository has been archived by the owner on Sep 3, 2022. It is now read-only.

build: Add commitzen and lint-staged #98

Closed
wants to merge 1 commit into from
Closed

Conversation

f2prateek
Copy link
Contributor

Adds commitzen as a dev dependency so we can write better commit messages. Also adds lint-staged as a dev dependency, which was overlooked from a previous commit when integrating Prettier.

Adds commitzen as a dev dependency so we can write better commit messages. Also adds `lint-staged` as a dev dependency, which was overlooked from a previous commit when integrating Prettier.
@f2prateek f2prateek requested review from fathyb, ccnixon and jlee9595 July 21, 2018 08:33
@codecov-io
Copy link

Codecov Report

Merging #98 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #98   +/-   ##
=======================================
  Coverage   98.58%   98.58%           
=======================================
  Files          11       11           
  Lines         636      636           
=======================================
  Hits          627      627           
  Misses          9        9

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 1187fc3...caa6d0b. Read the comment docs.

},
"config": {
"commitizen": {
"path": "./node_modules/cz-conventional-changelog"
Copy link
Contributor

@fathyb fathyb Jul 22, 2018

Choose a reason for hiding this comment

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

nit: I think "path": "cz-conventional-changelog" should be enough.

@@ -15,7 +15,8 @@
"lint": "eslint \"./{lib,test}/**/*.js\"",
"format": "prettier-eslint --write --list-different \"./{lib,test}/**/*.{js,json,md}\"",
"precommit": "lint-staged",
"np": "np --no-publish"
"np": "np --no-publish",
"commit": "git cz"
Copy link
Contributor

Choose a reason for hiding this comment

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

This means that precommit will be executed twice, the first time by npm before commit and the second time by husky after commit. See typicode/husky#99 for some workarounds.

@fathyb
Copy link
Contributor

fathyb commented Jul 22, 2018

Also adds lint-staged as a dev dependency, which was overlooked from a previous commit when integrating Prettier.

I'm not sure to understand this part. I don't see anything related to lint-staged in the changes.


I'm a big commitizen fan but I'm not sure if this is the most adapted tool for this project. It's an excellent tool to help new contributors onbard with a project commit convention but may just end up slowing down some users (eg. I'm familiar with this convention and use it everywhere but like to commit using tig to review changes as I commit).

While I think we should keep it for new users we should use something like commitlint and husky's commitmsg hook to ensure any commit made using any client/tool (like commitizen, tig, GitKraken, etc...) is conform to our convention. It could also help check that PRs are conform to the convention. What do you think?

@f2prateek
Copy link
Contributor Author

Re:lint-staged, this is because without adding it as a dev dependency, yarn commit will fail because the user may not have lint-staged installed. This likely wasn't exposed before because there was no reason to run yarn commit previously.

Down to try other tools here! My main goal is to be able to generate more friendly changelog from commit messages.

@fathyb
Copy link
Contributor

fathyb commented Jul 22, 2018

Re:lint-staged, this is because without adding it as a dev dependency, yarn commit will fail because the user may not have lint-staged installed. This likely wasn't exposed before because there was no reason to run yarn commit previously.

I meant that there is no changes about lint-staged in the PR git diff, it's already in the devDependencies in master. Just wanted to be sure it was intentional and not a base branch problem or something like that.

Copy link
Contributor

@fathyb fathyb left a comment

Choose a reason for hiding this comment

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

A remark about npm/husky hooks, but LGTM otherwise. Really like conventional commits 👍

@f2prateek
Copy link
Contributor Author

You're right - not sure why lint-staged wasn't setup locally, I must have not pulled from master properly. I'll update the PR Description.

@f2prateek
Copy link
Contributor Author

f2prateek commented Jul 23, 2018

While I think we should keep it for new users we should use something like commitlint and husky's commitmsg hook to ensure any commit made using any client/tool (like commitizen, tig, GitKraken, etc...) is conform to our convention. It could also help check that PRs are conform to the convention. What do you think?

@fathyb I think we should do these! I'll close this one for now, but feel free to send a PR to set those up.

@f2prateek f2prateek closed this Jul 23, 2018
@f2prateek f2prateek deleted the commitzen branch July 23, 2018 17:53
@fathyb fathyb mentioned this pull request Jul 24, 2018
2 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants