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

chore: add commit conventions doc #204

Merged
merged 1 commit into from
Jan 18, 2022

Conversation

mbokinala
Copy link
Contributor

taken from vitejs/vite

closes #190

taken from vitejs/vite

closes faker-js#190
@netlify
Copy link

netlify bot commented Jan 17, 2022

✔️ Deploy Preview for vigilant-wescoff-04e480 ready!

🔨 Explore the source changes: a9d1db3

🔍 Inspect the deploy log: https://app.netlify.com/sites/vigilant-wescoff-04e480/deploys/61e5ef102513010008dbbc78

😎 Browse the preview: https://deploy-preview-204--vigilant-wescoff-04e480.netlify.app

@import-brain import-brain requested a review from a team January 18, 2022 04:25
@import-brain
Copy link
Member

Going on a tangent here, but do we have a commit/PR message linting workflow set up on this repo? If not, we should probably set one up to enforce the commit conventions.

@import-brain import-brain added c: docs Improvements or additions to documentation c: chore PR that doesn't affect the runtime behavior labels Jan 18, 2022
@JessicaSachs
Copy link
Contributor

JessicaSachs commented Jan 18, 2022 via email

@import-brain
Copy link
Member

I’m not sure where the prettier/eslint discussion is right now. I know we have a commitmessage workflow. To verify commit messages, Shini put it together with a custom ./verifyCommit script inside of the scripts directory. This runs on a pre-commit hook. I find it a bit irksome because I think that’s going about it from the wrong side. I’d prefer to use a GitHub Status Check to enforce the commit message on the Pull Request and then ensure that all Pulls are squashed and merged. We do this at Cypress and it ensures you can push any kind of commit you want (merge commits, “wip”, “update”) and then the PR commit is the one that’s taken.

On Mon, Jan 17, 2022 at 11:26 PM Eric Cheng @.> wrote: Going on a tangent here, but do we have a commit/PR message linting workflow set up on this repo? If not, we should probably set one up to enforce the commit conventions. — Reply to this email directly, view it on GitHub <#204 (comment)>, or unsubscribe <github.com/notifications/unsubscribe-auth/AAVL4BHICO4TVODGULYUYI3UWTTXTANCNFSM5MFWVBPA> . You are receiving this because your review was requested.Message ID: @.>

The status check idea seems good.

@Shinigami92 What are your thoughts on this? Should we move to a status check?

@Shinigami92
Copy link
Member

image

This one IS the check!
It checks the name of the PR via https://github.com/faker-js/faker/blob/main/.github/semantic.yml

The verifyCommit file does the rest locally.

The setup is currently same like in https://github.com/vitejs/vite and works very well there.
When I contributed the PR #68, I just missed this file.

@Shinigami92 Shinigami92 merged commit 3c90061 into faker-js:main Jan 18, 2022
@import-brain
Copy link
Member

image

This one IS the check! It checks the name of the PR via main/.github/semantic.yml

The verifyCommit file does the rest locally.

The setup is currently same like in vitejs/vite and works very well there. When I contributed the PR #68, I just missed this file.

Ohhh I see, I was wondering what that check was. Thanks!

@mbokinala mbokinala deleted the commit-conventions branch January 18, 2022 16:53
Shinigami92 pushed a commit to MohdImran001/faker that referenced this pull request Jan 18, 2022
pkuczynski pushed a commit to pkuczynski/faker that referenced this pull request Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: chore PR that doesn't affect the runtime behavior c: docs Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add commit-convention.md
5 participants