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

docs(readme): describe commit conventions #2645

Merged
merged 2 commits into from
Mar 8, 2024
Merged

Conversation

hairyhum
Copy link
Contributor

@hairyhum hairyhum commented Feb 1, 2024

Change Overview

Add commit conventions discussed in #2582

Pull request type

Please check the type of change your PR introduces:

  • 🚧 Work in Progress
  • 🌈 Refactoring (no functional changes, no api changes)
  • 🐹 Trivial/Minor
  • πŸ› Bugfix
  • 🌻 Feature
  • πŸ—ΊοΈ Documentation
  • πŸ€– Test

@infraq infraq added this to In Progress in Kanister Feb 1, 2024
@hairyhum hairyhum marked this pull request as ready for review February 5, 2024 19:53
CONTRIBUTING.md Outdated

An explanation of the problem, providing context, and why the change is being
made.
[optional body]
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @hairyhum ,
Sorry, if I am mistaken. But I thought we discussed that this format should be followed in PR titles and desc.
What I had in mind for commits was a rule set like this

Commits should be of format

Title
Description
  • It's totally fine if you are able to convey the change in title itself, you don't necessarily have to provide desc.
  • Commit titles should not start with lower case. So add go mod check while init -> Add go mod check while init, same applies to PR titles as well
  • Title should imperative so use Add go lint instead of Added go lint, same applies to PR titles as well
  • Do not end title with period
  • Desc (if present) should specify why did you make that change
  • Don't assume reviewer understand the problem

This is very rough, but I think for commits we should have this kind of rules and for PR titles we can have the type and scope etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So your proposal is to have PR title/description convention instead of commit convention?

@hairyhum
Copy link
Contributor Author

@viveksinghggits i've tried to reword the requirements and give more context on the flow. Does that make sense?

@@ -89,16 +89,21 @@ to make reviews and retrospection easy. Use your git commits to provide context
for the reviewers, and the folks who will be reading the codebase in the months
Copy link
Contributor

Choose a reason for hiding this comment

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

this futurelearn link is not working anymore we should remove that.

CONTRIBUTING.md Outdated
Finalized commit messages should look similar to the following format:
We're trying to keep all commits in `master` to follow [conventional commits](https://www.conventionalcommits.org/en/v1.0.0/) format.
See [conventions](#commit-conventions) for more info on types and scopes.
We are using squash and merge approach to PRs which means that commit descriptions are generated from PR descriptions.
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not really sure if the desc for the commit, that is created in master when a PR is merged is, is created from PR desc. Instead I have seen that commit desc just has the other commits/their desc from the PR.
I was looking into this PR for example #2664 below is how this PR is merged
image

As we can see the commit message is PR title but the commit desc is the commits of the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant pr title. Updated.


```text
Short one line title
It's recommended to use conventional commits when strarting a PR, but follow-up commits in the PR don't have to follow the convention.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should also mention the points under 5 Steps to Write Better Commit Messages from this blog post https://www.freecodecamp.org/news/how-to-write-better-git-commit-messages/ somewhere here.
I really like the points where imperative mood is recommended and others.

Copy link
Contributor

@viveksinghggits viveksinghggits left a comment

Choose a reason for hiding this comment

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

LGTM

Kanister automation moved this from In Progress to Reviewer approved Mar 8, 2024
@mergify mergify bot merged commit 639f6ed into master Mar 8, 2024
14 checks passed
@mergify mergify bot deleted the conventional-commits branch March 8, 2024 18:51
Kanister automation moved this from Reviewer approved to Done Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants