| Message ID | afb80b8e9ee022cba9373f2191ee1619e5897b09.1651086288.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve MyFirstContribution's GitGitGadget section | expand |
Philippe Blain via GitGitGadget wrote: > From: Philippe Blain <levraiphilippeblain@gmail.com> > > The "Sending Patches via GitGitGadget" section mentions that the PR > title and description will be used as the cover letter, but does not > explain what is a cover letter or what should be included in it. > > Mention the purpose of the cover letter in that section, and give > examples for the title and description, leveraging the excerpt extracted > from the "Sending Patches with git send-email" section in the previous > commit. > > Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com> > --- > Documentation/MyFirstContribution.txt | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/Documentation/MyFirstContribution.txt b/Documentation/MyFirstContribution.txt > index 681bbefe9cd..96da32f7cef 100644 > --- a/Documentation/MyFirstContribution.txt > +++ b/Documentation/MyFirstContribution.txt > @@ -808,8 +808,20 @@ https://github.com/gitgitgadget/git and open a PR either with the "New pull > request" button or the convenient "Compare & pull request" button that may > appear with the name of your newly pushed branch. > > -Review the PR's title and description, as it's used by GitGitGadget as the cover > -letter for your change. When you're happy, submit your pull request. > +Review the PR's title and description, as they're used by GitGitGadget as the > +cover letter for your change. The cover letter describes your proposed > +contribution as a whole and is ideal to mention any context that might be > +useful for reviewers. The title of your PR should be something which > +succinctly covers the purpose of your entire topic branch, for example: > + > +---- > +Adding the 'psuh' command Typically I see patch series titles follow the same "imperative mood" as commit titles/messages (see 'Documentation/SubmittingPatches.txt'). I'm not sure whether that's a rule written down somewhere or just convention, but for the sake of consistency you might want to do something like: "Add the 'psuh' command" > +---- > + > +Your PR's description will used as the body of the cover letter. Including the line "Your PR's description..." is somewhat confusing to me as a first-time reader, since I was interpreting this section to be the *verbatim* text of the pull request title & description. If this *is* meant to be that description, then the note about the PR description can be removed. That point is also mentioned above, so it's probably not needed here anyway. > +include::MyFirstContribution-coverletter.txt[] > + > +When you're happy, submit your pull request. > > [[run-ci-ggg]] > === Running CI and Getting Ready to Send
Hi Victoria, Le 2022-04-27 à 16:43, Victoria Dye a écrit : > Philippe Blain via GitGitGadget wrote: >> +---- >> +Adding the 'psuh' command > > Typically I see patch series titles follow the same "imperative mood" as > commit titles/messages (see 'Documentation/SubmittingPatches.txt'). I'm not > sure whether that's a rule written down somewhere or just convention, but > for the sake of consistency you might want to do something like: > > "Add the 'psuh' command" > I fully agree. I just copied the existing patch series title from the git-send-email section further down. I think it would make sense to change this to also using the imperative mood just like commit messages in a preparatory commit. I'll do that. >> +---- >> + >> +Your PR's description will used as the body of the cover letter. > > Including the line "Your PR's description..." is somewhat confusing to me as > a first-time reader, since I was interpreting this section to be the > *verbatim* text of the pull request title & description. If this *is* meant > to be that description, then the note about the PR description can be > removed. That point is also mentioned above, so it's probably not needed > here anyway. I'm not exactly sure what you mean. I meant that the description of the PR will be used as the body of the cover letter... Thanks for having a look! Philippe.
Philippe Blain wrote: > Hi Victoria, > > Le 2022-04-27 à 16:43, Victoria Dye a écrit : >> Philippe Blain via GitGitGadget wrote: > >>> +---- >>> +Adding the 'psuh' command >> >> Typically I see patch series titles follow the same "imperative mood" as >> commit titles/messages (see 'Documentation/SubmittingPatches.txt'). I'm not >> sure whether that's a rule written down somewhere or just convention, but >> for the sake of consistency you might want to do something like: >> >> "Add the 'psuh' command" >> > > I fully agree. I just copied the existing patch series title from > the git-send-email section further down. I think it would make sense > to change this to also using the imperative mood just like commit messages > in a preparatory commit. I'll do that. > >>> +---- >>> + >>> +Your PR's description will used as the body of the cover letter. >> >> Including the line "Your PR's description..." is somewhat confusing to me as >> a first-time reader, since I was interpreting this section to be the >> *verbatim* text of the pull request title & description. If this *is* meant >> to be that description, then the note about the PR description can be >> removed. That point is also mentioned above, so it's probably not needed >> here anyway. > > I'm not exactly sure what you mean. I meant that the description of the PR > will be used as the body of the cover letter... > Sorry for being unclear. What I meant was that, the way this part of the document reads (to me) right now, it looks like the recommendation is to create a pull request with the title & description: Title: Adding the 'psuh' command Description: Your PR's description will [be] used as the body of the cover letter. <insert content from MyFirstContribution-coverletter.txt> That is, the line "Your PR description will..." is the first line of the description (which I don't think was your intention). That said, upon a more holistic re-read, I've found a few more things that are confusing/oddly phrased. I'll try re-reviewing (part of) the patch below, with (hopefully) more direct/concrete feedback this time: > -Review the PR's title and description, as it's used by GitGitGadget as the cover > -letter for your change. When you're happy, submit your pull request. > +Review the PR's title and description, as they're used by GitGitGadget as the > +cover letter for your change. The cover letter describes your proposed > +contribution as a whole and is ideal to mention any context that might be s/is ideal to/should(?) > +useful for reviewers. The title of your PR should be something which > +succinctly covers the purpose of your entire topic branch, for example: > + > +---- > +Adding the 'psuh' command > +---- Other than the "Adding" -> "Add" change, this is good - the example title will be formatted as a monospaced block, clearly separating it from the advice/context preceding it. > + > +Your PR's description will used as the body of the cover letter. > +include::MyFirstContribution-coverletter.txt[] > + Conversely, this section has no visible separation between the context ("Your PR's description...") and the example ("include::MyFirstContribution-coverletter.txt[]"), hence my confusion earlier. To parallel the title example, maybe you could use similar monospace formatting for the example? E.g.: + +Your PR's description will used as the body of the cover letter. + +---- +include::MyFirstContribution-coverletter.txt[] +---- + One thing I noticed on my re-read is that the context you provide for the title ("The title of your PR should be something...") isn't quite paralleled in the context for the description ("Your PR's description..."). The former talks about *how* you should write your cover letter title, whereas the latter simply states that the PR description will become the cover letter's content. For added context, then, you might want to describe how the cover letter content should be written, e.g.: + +Your PR's description will be used as the body of the cover letter and +should explain the purpose of the series, for example: + +---- +include::MyFirstContribution-coverletter.txt[] +---- + (note the s/will used/will be used) Apologies for all the picky grammar comments - this series includes a lot of really helpful information for new contributors, and I really appreciate your work here! Hopefully my feedback is a bit more clear this time (and is relatively straightforward to implement). Thanks! > Thanks for having a look! > > Philippe.
Hi Victoria, Le 2022-04-29 à 12:27, Victoria Dye a écrit : > Philippe Blain wrote: >> Hi Victoria, >> >> Le 2022-04-27 à 16:43, Victoria Dye a écrit : >>> Philippe Blain via GitGitGadget wrote: >> >>>> +---- >>>> +Adding the 'psuh' command >>> >>> Typically I see patch series titles follow the same "imperative mood" as >>> commit titles/messages (see 'Documentation/SubmittingPatches.txt'). I'm not >>> sure whether that's a rule written down somewhere or just convention, but >>> for the sake of consistency you might want to do something like: >>> >>> "Add the 'psuh' command" >>> >> >> I fully agree. I just copied the existing patch series title from >> the git-send-email section further down. I think it would make sense >> to change this to also using the imperative mood just like commit messages >> in a preparatory commit. I'll do that. >> >>>> +---- >>>> + >>>> +Your PR's description will used as the body of the cover letter. >>> >>> Including the line "Your PR's description..." is somewhat confusing to me as >>> a first-time reader, since I was interpreting this section to be the >>> *verbatim* text of the pull request title & description. If this *is* meant >>> to be that description, then the note about the PR description can be >>> removed. That point is also mentioned above, so it's probably not needed >>> here anyway. >> >> I'm not exactly sure what you mean. I meant that the description of the PR >> will be used as the body of the cover letter... >> > > Sorry for being unclear. What I meant was that, the way this part of the > document reads (to me) right now, it looks like the recommendation is to > create a pull request with the title & description: > > Title: > Adding the 'psuh' command > Description: > Your PR's description will [be] used as the body of the cover > letter. <insert content from MyFirstContribution-coverletter.txt> > > That is, the line "Your PR description will..." is the first line of the > description (which I don't think was your intention). > > That said, upon a more holistic re-read, I've found a few more things that > are confusing/oddly phrased. I'll try re-reviewing (part of) the patch > below, with (hopefully) more direct/concrete feedback this time: > >> -Review the PR's title and description, as it's used by GitGitGadget as the cover >> -letter for your change. When you're happy, submit your pull request. >> +Review the PR's title and description, as they're used by GitGitGadget as the >> +cover letter for your change. The cover letter describes your proposed >> +contribution as a whole and is ideal to mention any context that might be > > s/is ideal to/should(?) > >> +useful for reviewers. The title of your PR should be something which >> +succinctly covers the purpose of your entire topic branch, for example: >> + >> +---- >> +Adding the 'psuh' command >> +---- > > Other than the "Adding" -> "Add" change, this is good - the example title > will be formatted as a monospaced block, clearly separating it from the > advice/context preceding it. > >> + >> +Your PR's description will used as the body of the cover letter. >> +include::MyFirstContribution-coverletter.txt[] >> + > > Conversely, this section has no visible separation between the context > ("Your PR's description...") and the example > ("include::MyFirstContribution-coverletter.txt[]"), hence my confusion > earlier. > > To parallel the title example, maybe you could use similar monospace > formatting for the example? E.g.: > > + > +Your PR's description will used as the body of the cover letter. > + > +---- > +include::MyFirstContribution-coverletter.txt[] > +---- > + > > One thing I noticed on my re-read is that the context you provide for the > title ("The title of your PR should be something...") isn't quite paralleled > in the context for the description ("Your PR's description..."). The former > talks about *how* you should write your cover letter title, whereas the > latter simply states that the PR description will become the cover letter's > content. For added context, then, you might want to describe how the cover > letter content should be written, e.g.: > > + > +Your PR's description will be used as the body of the cover letter and > +should explain the purpose of the series, for example: > + > +---- > +include::MyFirstContribution-coverletter.txt[] > +---- > + > > (note the s/will used/will be used) > > Apologies for all the picky grammar comments - this series includes a lot of > really helpful information for new contributors, and I really appreciate > your work here! Hopefully my feedback is a bit more clear this time (and is > relatively straightforward to implement). Thanks for your feedback. I'll send an updated and expanded version soon and I think I've adressed the different things you noted. Cheers, Philippe.
diff --git a/Documentation/MyFirstContribution.txt b/Documentation/MyFirstContribution.txt index 681bbefe9cd..96da32f7cef 100644 --- a/Documentation/MyFirstContribution.txt +++ b/Documentation/MyFirstContribution.txt @@ -808,8 +808,20 @@ https://github.com/gitgitgadget/git and open a PR either with the "New pull request" button or the convenient "Compare & pull request" button that may appear with the name of your newly pushed branch. -Review the PR's title and description, as it's used by GitGitGadget as the cover -letter for your change. When you're happy, submit your pull request. +Review the PR's title and description, as they're used by GitGitGadget as the +cover letter for your change. The cover letter describes your proposed +contribution as a whole and is ideal to mention any context that might be +useful for reviewers. The title of your PR should be something which +succinctly covers the purpose of your entire topic branch, for example: + +---- +Adding the 'psuh' command +---- + +Your PR's description will used as the body of the cover letter. +include::MyFirstContribution-coverletter.txt[] + +When you're happy, submit your pull request. [[run-ci-ggg]] === Running CI and Getting Ready to Send