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(CONTRIBUTING.md): enforce commit guidelines #10748

Merged
merged 4 commits into from Oct 10, 2022

Conversation

thunder-coding
Copy link
Member

@thunder-coding thunder-coding commented May 20, 2022

Requesting reviews from @Grimler91 @xtkoba @buttaface @MrAdityaAlok.

Feel free to ping any other organization member who'll be affected by this change, or suggest any changes!

TODOs:-

  • Document new commit guidelines.
  • Resolve changes requested by team members and contributors.
  • Adhere auto update commits to current guidelines.
  • Add a script which can be used by contributors with Git Hooks which can lint commit messages before commiting.
  • Add a commit lint step to CI.

@xtkoba
Copy link
Contributor

xtkoba commented May 20, 2022

I don't like this.

Copy link
Member

@Grimler91 Grimler91 left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me, thanks for formalising this!

We can add a lint script that checks the commit message style as well, so that user has a chance to fix style issues without maintainers nagging about it

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
@Grimler91
Copy link
Member

I don't like this.

Writing guidelines on how commits should preferably be written should be a good idea in any case.
Is your preference the format package: some description?

@xtkoba
Copy link
Contributor

xtkoba commented May 20, 2022

Writing guidelines on how commits should preferably be written should be a good idea in any case.

This is true. We have seen a bunch of PRs of which the subject is not comprehensive.

Yes I prefer package: Some description and new package: pkgname, because I believe they are comprehensive and easy to write.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@thunder-coding
Copy link
Member Author

Yes I prefer package: Some description and new package: pkgname, because I believe they are comprehensive and easy to write.

I decided to draft this proposal in order to make commit messages more uniform and make history easy to browse. This commit guideline is inspired by Node.js and Arch Linux's commit messages. In case you have any better idea in your mind, feel free to share. After all, it doesn't matter how good the proposal is unless all (or most) of the existing collaborators agree to it.

@xtkoba
Copy link
Contributor

xtkoba commented May 20, 2022

My proposal:

  • If a commit is about to modify a single package, then the subject should contain the package name.

And only this. Unnecessarily complex rules are just hard to obey.

@truboxl
Copy link
Contributor

truboxl commented May 21, 2022

How long should commit message be?
I still prefer xtkoba's way as well since the proposal add quite a bit characters and not fit 50 chars for long package names...

@agnostic-apollo
Copy link
Member

The suggested rules by @thunder-coding look good to me too. They are following the conventional commit design, which is used by lot of FOSS projects now and also the termux apps repos mostly. Deciding scope as package name is easier here than app components though.

https://www.conventionalcommits.org/en/v1.0.0

CONTRIBUTING.md Outdated Show resolved Hide resolved
@stale
Copy link

stale bot commented Jul 26, 2022

This issue/PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix Issue won't be fixed. label Jul 26, 2022
@2096779623 2096779623 added not stale and removed wontfix Issue won't be fixed. labels Jul 26, 2022
@2096779623
Copy link
Member

@thunder-coding #11281

@thunder-coding
Copy link
Member Author

Lol it's the typo I made earlier, which I fixed somewhere between. It's upgpkg only standing for upgrade package.

Anyways it's inspired by Arch's commit messages used in their VCS.

@thunder-coding
Copy link
Member Author

About adding scripts to lint commit messages, I don't think that's the priority for me now. In case anyone is interested in it, I would be very glad. Atleast I don't expect me to work on it. In case everything seems good, and I get a approving review, I'll format the markdown file, (possibly affecting other lines than the changed ones too), and merge it.

@thunder-coding
Copy link
Member Author

Let's finally get this merged. I see a lot do new PRs following similar guidelines, so making it official from semioffical might be worth it :)

CONTRIBUTING.md Outdated Show resolved Hide resolved
@agnostic-apollo
Copy link
Member

adapt commit messages to new guiselines

What's that? Second typo ;)

@thunder-coding
Copy link
Member Author

adapt commit messages to new guiselines

What's that? Second typo ;)

So you've been spying on the commit log where you saw the old typo fix :) Anyways fixed. Thanks\

@agnostic-apollo
Copy link
Member

So you've been spying on the commit log where you saw the old typo fix :) Anyways fixed. Thanks\

When a dev opens a pull request about proper commit messages and then pushes commits with the title adapt commit mwssages to new guiselines, there is obviously fun to be had ;) And welcome.

@thunder-coding
Copy link
Member Author

I'll merge this PR tommorow if there are no changes requested by anyone else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants