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

Put issue numbers in PR description, not in PR title #8735

Closed
tianyizheng02 opened this issue May 15, 2023 · 6 comments · Fixed by #8833
Closed

Put issue numbers in PR description, not in PR title #8735

tianyizheng02 opened this issue May 15, 2023 · 6 comments · Fixed by #8833
Labels
enhancement This PR modified some existing files

Comments

@tianyizheng02
Copy link
Contributor

Feature description

Currently the PR template includes the following checklist item:

If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.

I think it's better to ask contributors to put issue numbers in the PR description rather than the commit message for the following reasons:

  1. The vast majority of contributors to this repo (myself included) don't put closing keywords in commit messages. In my experience, the norm on GitHub is to put them in the PR description.

  2. Since contributors typically make their commits in their local git repos and not directly on GitHub, it's less convenient to include the issue number because git can't autocomplete GitHub issue numbers.

  3. A PR that resolves an issue often includes multiple commits, so no single commit single-handedly resolves the issue. Thus, putting the issue number in the PR description makes more sense IMO.

  4. From the GitHub docs:

    You can also use closing keywords in a commit message. The issue will be closed when you merge the commit into the default branch, but the pull request that contains the commit will not be listed as a linked pull request.

    Therefore, even if one puts the issue number with a closing keyword in a commit, GitHub won't automatically link the PR that it's a part of, which makes it slightly harder to refer back to the original issue.

We can also link to the GitHub docs (see above) in the checklist item to help contributors use the closing keywords correctly.

@tianyizheng02 tianyizheng02 added the enhancement This PR modified some existing files label May 15, 2023
@tianyizheng02
Copy link
Contributor Author

@dhruvmanila @cclauss What do you think?

@dhruvmanila
Copy link
Member

Yeah I agree. Although I'm not sure how to enforce it. We could add it in Contributing Guidelines but vast majority of contributors won't be reading it. There's a check in the PR description to add any closing keywords if the PR is addressing an issue. Any suggestions?

@tianyizheng02
Copy link
Contributor Author

@dhruvmanila I think it's still best to add it to the contributing guidelines for the few contributors who do read it (and those who don't read it won't be able to feign ignorance). As for the PR check, I think we could clarify the wording to emphasize where to put the issue number:

  • If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}. I have included the issue number with a closing keyword in the description: "Fixes #ISSUE-NUMBER"

or something like this.

To address what @cclauss brought up, we might want to emphasize (both in the contributing docs and the PR checklist) that PRs should have descriptive titles.

@tianyizheng02
Copy link
Contributor Author

As for enforcement, maybe we could have the algorithms-keeper bot check the PR description for an issue number and closing keyword if the contributor checked the "resolves an issue" checkbox? I'm not sure how exactly that'd be implemented. Other than that, I think it'd have to be enforced on a case-by-case basis.

@cclauss cclauss changed the title Put issue numbers in PR description, not commit message Put issue numbers in PR description, not in PR title May 28, 2023
@cclauss
Copy link
Member

cclauss commented May 28, 2023

I changed the title of this issue because a commit message can be multiple lines. In this case, the first line is the pull request title and the remaining lines are the pull request body (or description). We want the issue #number in the body, not in the title so that it is clickable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This PR modified some existing files
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants