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

Change suggestion in CONTRIBUTING.md to avoid spam #93468

Merged
merged 1 commit into from
Jun 24, 2024

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Jun 22, 2024

Having an issue tagged in a commit message spams the issue thread with notifications, which can be especially noisy when new contributors make a lot of updates or rebases.

The note serves little use in the commit message itself

Edit: for those who might not know what I'm talking about, here's one example
Screenshot

Having an issue tagged in a commit message spams the issue thread with
notifications, which can be especially noisy when new contributors make
a lot of updates or rebases.
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

I also agree that adding a notice so the pr description (not in the title, or the commit message) is unnecessary.

@akien-mga
Copy link
Member

akien-mga commented Jun 22, 2024

I'm not sure I agree. It's valuable information IMO to have the fixed issue mentioned in the commit message, so that you can go straight from a commit to the issue it's fixing. Additionally, the PR description is inferred from the commit message, so having the "Fixes #..." line in the body of the commit message ensures that it's also properly linked in the PR.

The spam on issues is honestly a GitHub bug they should fix. They could group multiple notifications together, or remove the ones originated from commits which have been garbage collected eventually because not merged.

But it's not a hill I'd die on, I see the downsides too when combined with our rebase/amend heavy workflow.

@AThousandShips
Copy link
Member Author

AThousandShips commented Jun 22, 2024

Those are valid points, personally I find it not too long of a process to go from the commit to the PR and to the issue

But there's also the fact that the PR is often updated with more linked issues and it's usually not reflected in the commit message, so I think to make it useful to allow it we'd need to ensure contributors match up in their commits after such changes

@akien-mga
Copy link
Member

I think we can remove the recommendation, but I wouldn't make it a policy to ask people to remove issue references from their commit messages, as this is common commit etiquette and adds unnecessary friction IMO.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Jun 24, 2024
@AThousandShips
Copy link
Member Author

That sounds like a good balance to me

@akien-mga
Copy link
Member

I think we can remove the recommendation, but I wouldn't make it a policy to ask people to remove issue references from their commit messages, as this is common commit etiquette and adds unnecessary friction IMO.

(Unless we notice that a contributor is rebasing their PR multiple times per day leading to a lot of spam - in this case I think it's fine to suggest they could remove the issue reference, at least until they're done with their draft.)

@akien-mga akien-mga merged commit 1cc83ad into godotengine:master Jun 24, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@AThousandShips AThousandShips deleted the contributing_improve branch June 24, 2024 09:19
@AThousandShips
Copy link
Member Author

Thank you!

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

Successfully merging this pull request may close these issues.

3 participants