Skip to content

Improve "Commit Messages" section in committing.rst #199

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

Closed
berkerpeksag opened this issue May 15, 2017 · 4 comments
Closed

Improve "Commit Messages" section in committing.rst #199

berkerpeksag opened this issue May 15, 2017 · 4 comments

Comments

@berkerpeksag
Copy link
Member

It would be great if we could tweak the section at http://cpython-devguide.readthedocs.io/committing.html#commit-messages a bit. For example,

  1. It doesn't include (GH-NNNN) suffix in the first line:

    bpo-42: the spam module is now more spammy. (GH-1234)
    
  2. It doesn't mention about stripping unnecessary parts when doing the merge (see python/cpython@6924ed5 for example)

  3. It doesn't talk about what to do with backport PRs (e.g. should we drop the backport PR number from the title?)
    Currently, we do have:

    bpo-42: the spam module is now more spammy. (GH-1234) (#4242)
    

    #4242 is the backport PR here, but it's rarely useful and it can be seen by using the GitHub UI.

    I suggest stripping it and only keep the original PR number:

    bpo-42: the spam module is now more spammy. (GH-1234)
    

    Another question is that should we add [X.Y] to commit messages too or should we only add it to the PR title?

  4. It doesn't talk about what to do with GH-NNNN annotations when the first line of the commit message is too long. See python/cpython@edef358 for example. Should we move the annotation to the end of the commit message in this case? For example:

    bpo-29196: Removed old-deprecated classes Plist, Dict and _InternalDict
    in the plistlib module.
    
    Dict values in the result of functions readPlist() and readPlistFromBytes()
    are now exact dicts.
    
    Pull request: GH-488
    
@Mariatta
Copy link
Member

Sounds good.

Some comments:

It doesn't mention about stripping unnecessary parts when doing the merge (see python/cpython@6924ed5 for example)

It's briefly mentioned in here, but I think makes more sense now to move this into Committing section.

should we add [X.Y] to commit messages too or should we only add it to the PR title?

I'm inclined to keep the [X.Y] in the commit message, only because cherry_picker.py is doing this by default. I also understand that not everyone uses it, and also cherry_picker.py is supposed to be temporary until a bot is in place. So I won't oppose omitting it from the commit message.

@brettcannon
Copy link
Member

I took a small poll here at the PyCon US sprints and basically everyone uniformly said "keep the branch label" and "don't care about the listed PRs", so basically just keep the title intact compared to what we have people specify for PRs currently.

@Mariatta
Copy link
Member

@berkerpeksag I think everything has been addressed, so I'm closing this. Please reopen if it turns out that I miss something. Thanks.

@berkerpeksag
Copy link
Member Author

@Mariatta thank you, it looks pretty good to me. I missed PR #275. I've just opened #288 for my tweaks.

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

No branches or pull requests

3 participants