Clarify "Co-authored-by" usage in PRs

[aeros]

As brought up in a Discourse committers topic, the devguide could be more clear about Pull Request attribution on GitHub.

Specifically, in the fourth paragraph of "Handling Others' Code", the Co-authored-by: feature of GitHub is mentioned, but the use case of it is not defined.

Since GitHub will automatically attribute the primary author of the PR when using "squash and merge" or the "automerge", the Co-authored-by: is useful to add just prior to merging for anyone that significantly contributed to the PR, either via review or direct suggested changes. When using the GitHub UI to directly commit a suggested change to the PR, a Co-authored-by: line is automatically added to the commit message. Otherwise though, it has to be done manually for people other than the author to be attributed for contributing to the PR.

With this being a rather substantial part of the workflow for GitHub PRs, it seems to be worthwhile to mention when Co-authored-by: should be used rather than mentioning the feature without elaborating further.

[cjerdonek]

In addition to the "Co-authored-by," the "Patch by" instructions also need to be clarified. Like, one sentence says this:

Third, ensure the patch is attributed correctly with the contributor’s name in Misc/ACKS if they aren’t already there (and didn’t add themselves in their patch) and by mentioning “Patch by ” in the Misc/NEWS.d entry and the check-in message.

For the second part, it seems like "Patch by" is redundant in the commit message if they're already the author of the first commit. It should also clarify whether "Patch by" should be added to the NEWS entry if the Git history already has them as the primary author.

[cjerdonek]

I would also check more widely before saying that reviewers should be credited in "Co-authored-by." I had never heard of that suggestion before today.

[aeros]

I would also check more widely before saying that reviewers should be credited in "Co-authored-by." I had never heard of that suggestion before today.

Specifically, I meant that if a reviewer suggested a specific change, and it was added to the PR. In the GitHub UI when directly committing a suggestion, it automatically adds the person who made the suggestion to the "Co-authored-by:". So, I think it makes sense to do this manually if the author directly adds a change suggested by the reviewer. In a case where the reviewer makes a suggestion that does not get incorporated into the PR, it wouldn't make sense to include them in the "Co-authored-by:".

But you're right that we should verify this more widely before suggesting it in the devguide, I'll try to make a separate thread to get feedback on this when I can find the spare cycles to do so.

[cjerdonek]

Specifically, I meant that if a reviewer suggested a specific change, and it was added to the PR.

I don't know. That doesn't seem right to me. That would basically mean that anytime a reviewer made a suggestion on a PR that the author agreed to, they'd need to be added as a co-author. That seems like a significant broadening of the definition. Like, if you think about someone who writes a book, the book's editor might do a ton of work on making suggestions, but they're not listed as a co-author of the book.

On this point,

In the GitHub UI when directly committing a suggestion, it automatically adds the person who made the suggestion to the "Co-authored-by:".

If the suggestion is small, I would actually suggest the reverse and remove the Co-author attribution. (That's what I did when I made some small changes to someone else's PR before committing.) Just because GitHub's UI does something, I don't think we need to adopt that.

[aeros]

That would basically mean that anytime a reviewer made a suggestion on a PR that the author agreed to, they'd need to be added as a co-author. That seems like a significant broadening of the definition. Like, if you think about someone who writes a book, the book's editor might do a ton of work on making suggestions, but they're not listed as a co-author of the book.

That's a great point, I honestly hadn't thought about it that way. Perhaps then it should be based more on the significance of the suggestion(s) with respect to the entire PR. So, if a suggestion was made that had a fundamental impact on the overall feature, bug fix, behavior change, etc. a Co-authored-by: should be considered, but not for smaller ones (such as Misc/NEWS fixes, typos, and other minor issues).

For my own authored PRs, I've had a tendency to be more liberal with regards to giving Co-authored-by credit more recently when others provide good suggestions that have an impact on the PR. I find that it somewhat helps to encourage collaboration, and in general I've found that reviews are frequently underrated in significance (especially from non-core devs), so I'm glad to encourage it as best I can.

That being said, I can definitely understand the issue that it may be unfairly giving away too much credit to minor suggestions. This definitely seems like something we need to have a wider discussion about with regards to appropriate usage.

If the suggestion is small, I would actually suggest the reverse and remove the Co-author attribution. (That's what I did when I made some small changes to someone else's PR before committing.) Just because GitHub's UI does something, I don't think we need to adopt that.

I'm not sure if this is something that any other core developers already do, but after some consideration I agree that it would probably make sense to manually remove the Co-authored-by for minor suggestions when GitHub automatically adds it.

I may have given a bit too much weight to the fact that it happens to be a feature, and not adequately considered that a different policy might make more sense for us. Thanks for the feedback, I mostly agree with the points you made.

[terryjreedy]

I think it depends on the magnitude of the change. I just reworded a one-sentence news item and removed the co-author note. On the other hand, I have often so thoroughly revised a PR that in the past I added myself as co-author.

[Mariatta]

I would leave this to each committer's discretion. This is a GitHub-wide feature, I don't think we need to come up with our own rule on how to best use this feature.

[CAM-Gerlach]

For the second part, it seems like "Patch by" is redundant in the commit message if they're already the author of the first commit. It should also clarify whether "Patch by" should be added to the NEWS entry if the Git history already has them as the primary author.

FWIW, as a new(ish, I contributed a couple other minor changes a year or two back) contributor to CPython, it was pretty difficult to figure out the NEWS workflow in general, and this point in particular (in fact, I wasn't sure on it until I read this issue). In a details section, I've included a narrative description of my experience, in case its helpful in understanding from the beginner perspective the sources of confusion and how it can hopefully be improved. If this is tangential to the main questions raised by this PR, I'm happy to move this comment to a new issue.

Step 8 of the Quick reference, regarding adding a NEWS entry, gives a (appropriately) brief description about how to use blurb without mentioning what to put in the entry, and while it says "Please read more about blurb in documentation", the linked page doesn't mention blurb or the NEWS file at all aside from listing it as a required dependency. Presumably, this should be fixed to point to something more relevant, perhaps the blurb docs or the Updating NEWS section of Accepting PRs, but that's a separate issue.

I noticed the Crediting section of the PR workflow guide states "Non-trivial contributions are credited in the Misc/ACKS file (and, most often, in a contribution’s news entry as well).", but with no mention of the form or criteria for whether this should be included. After some effort (since it was buried in another repo), I found the blurb readme which gave more detail about using the tool, but provided little detail and no example of what the prose of the NEWS entry should look like, and nothing on the "patch/contributed by" line. Finally, I resorted to simply looking at other extant NEWS entries; some had contributor names and some didn't, but since looking back at my previous PR I was requested to add it there, I ended up doing so (I had done so unprompted for the What's New entry, since unlike the NEWS, essentially every one had a "contributed by" credit).

The accepting pull requests page was the one place that seems to be a great explanation of how to write a NEWS entry, including the credit line as well as syntax (e.g. :func: directives), formatting, etc and an actual example, and would have addressed my confusion and made it clear what I needed to do. However, I never found it at the time, since it was part of a page oriented toward maintainers rather than contributors.

TL;DR: The Updating news and what's new section of the Accepting pull requests page, presumably rewritten by @cjerdonek since this issue was created), does seem to answer this question and reflect the above, and it was the one resource I found helpful. Therefore, I suggest it, or a simplified version written with contributors rather than maintainers in mind, be linked somewhere that the average user is likely to find it. An ideal spot might be Step 8 of the quick reference describing adding a NEWS entry (since the current link in that step points to the "documenting" section, which doesn't actually refer to NEWS entries at all), as well as at least mentioning and linking it in the Documenting section and/or in the PR lifecycle section. Happy to contrib a PR if desired.