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

summarize semantic commit requirements #12665

Merged
merged 3 commits into from May 2, 2018

Conversation

Projects
None yet
6 participants
@zeke
Member

zeke commented Apr 19, 2018

This PR represents a first step toward a "conventional commits" requirement on electron/electron.

Rather than requiring every single commit message to follow a convention, the approach here is to require at least one semantic commit message in the PR, or a semantic PR title.

We've been using the https://github.com/apps/semantic-pull-request probot app on electron/i18n for a few weeks, and it's worked well. If a PR is ready to go but doesn't yet meet the semantic requirements, a maintainer can change the PR title to get checks passing, then merge.

Feedback welcome!

TODO:

  • Document semantic commit requirements in CONTRIBUTING (so we can link to them)
  • Add example commit messages
  • Change "must" to something more polite.

@zeke zeke requested a review from electron/electrocats as a code owner Apr 19, 2018

@zeke zeke requested review from jkleinsc, codebytere, ckerr and alexeykuzmin Apr 19, 2018

Show outdated Hide outdated .github/config.yml
@jkleinsc

Overall I think this good, though as @ckerr mentioned, I think examples would be helpful.

Show outdated Hide outdated .github/config.yml
Show outdated Hide outdated .github/config.yml
Show outdated Hide outdated .github/config.yml
@codebytere

This comment has been minimized.

Show comment
Hide comment
@codebytere

codebytere Apr 19, 2018

Member

I'd agree that examples would be awesome, but i think it might be easier for brevity of comment to add the examples to contribution docs somewhere and link to them?

Member

codebytere commented Apr 19, 2018

I'd agree that examples would be awesome, but i think it might be easier for brevity of comment to add the examples to contribution docs somewhere and link to them?

@zeke

This comment has been minimized.

Show comment
Hide comment
@zeke

zeke Apr 19, 2018

Member

add the examples to contribution docs somewhere and link to them

Yessss. Definitely want to have this documented so we can link to it as needed, especially because the message here only applies to first-timers, not the hundreds of people who have already landed PRs on electron. 👍

Started a TODO list up top ☝️

Member

zeke commented Apr 19, 2018

add the examples to contribution docs somewhere and link to them

Yessss. Definitely want to have this documented so we can link to it as needed, especially because the message here only applies to first-timers, not the hundreds of people who have already landed PRs on electron. 👍

Started a TODO list up top ☝️

@zeke zeke requested a review from electron/docs as a code owner Apr 20, 2018

@zeke

This comment has been minimized.

Show comment
Hide comment
@zeke

zeke Apr 20, 2018

Member

I made some updates based on all the feedback. This is ready for 👀 again.

Member

zeke commented Apr 20, 2018

I made some updates based on all the feedback. This is ready for 👀 again.

@zeke zeke changed the title from [WIP] summarize semantic commit requirements to summarize semantic commit requirements Apr 20, 2018

@codebytere

Latest updates looks great, with the examples you added i think this is ready to go 🚀

@alexeykuzmin

This comment has been minimized.

Show comment
Hide comment
@alexeykuzmin

alexeykuzmin Apr 21, 2018

Contributor

I like new examples in the present tense much more than the old ones in the past tense.

New:

Examples of commit messages with semantic prefixes:

  • fix: don't overwrite prevent_default if default wasn't prevented
  • feat: add app.isPackaged() method

Old:

Examples:

  • updated osx build documentation for new sdk
  • fixed typos in atom_api_menu.h
Contributor

alexeykuzmin commented Apr 21, 2018

I like new examples in the present tense much more than the old ones in the past tense.

New:

Examples of commit messages with semantic prefixes:

  • fix: don't overwrite prevent_default if default wasn't prevented
  • feat: add app.isPackaged() method

Old:

Examples:

  • updated osx build documentation for new sdk
  • fixed typos in atom_api_menu.h
@alexeykuzmin
  1. The first line should:
  • contain a short description of the change (preferably 50 characters or less,
    and no more than 72 characters)
    ...
  1. Wrap all other lines at 72 columns.

Are we going to drop line width requirement?
I guess a lot of people work with git in a console, and long commit messages just create a mess.
So I think the requirement should be kept and probably even enforced by a pre-commit hooks and CI checks (bots can work too).

@zeke

This comment has been minimized.

Show comment
Hide comment
@zeke

zeke Apr 30, 2018

Member

@alexeykuzmin I restored the line-width info. 👍

Member

zeke commented Apr 30, 2018

@alexeykuzmin I restored the line-width info. 👍

@ckerr

ckerr approved these changes May 2, 2018

I'm fine with this change as-is.

Since this does change the team's workflow, and since not everyone was at the Monday morning kickoff meeting, I'm going to ping #core asking for people to weigh in before merging.

@zcbenz

zcbenz approved these changes May 2, 2018

@ckerr ckerr merged commit 1957eb9 into master May 2, 2018

9 of 10 checks passed

ci/circleci: electron-osx-x64 Your tests failed on CircleCI
Details
WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-mas-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@ckerr ckerr deleted the semantic-first-pr branch May 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment