| Message ID | 0a5e027af3c0b61f9ab9f3c66e73b7a769eef7fd.1696983695.git.maxim.cournoyer@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#66436,v2] doc: Add some guidelines for reviewing. | expand |
Hi Maxim, Maxim Cournoyer <maxim.cournoyer@gmail.com> writes: > +@cindex LGTM, Looks Good To Me > +When you deem the proposed change adequate and ready for inclusion > +within Guix, the following well understood/codified @acronym{LGTM, Looks > +Good To Me} phrases should be used to sign off as a reviewer, meaning > +you have reviewed the change and that it looks good to you: > + > +@itemize > +@item > +If the @emph{whole} series (containing multiple commits) looks good to > +you, reply with a @samp{This series LGTM!} to the cover page if it has > +one, or to the last patch of the series otherwise. > + > +@item > +If you instead want to mark a @emph{single commit} as reviewed (but not > +the whole series), simply reply with @samp{LGTM!} to that commit > +message. > +@end itemize Some mbox -> patches tools (like b4) are able to automatically apply trailers like "Reviewed-by: some ident <...>" found as replies to patch series [1]. I also like the very clear meaning of it: there's no way you would type this without meaning "Yes, I officially vouch that this patch series has been reviewed by me". Maybe this could be codified here? Otherwise, seem good to me. [1] `b4 am`'s -t option, see https://b4.docs.kernel.org/en/latest/maintainer/am-shazam.html Best,
Hello, Josselin Poiret <dev@jpoiret.xyz> writes: > Hi Maxim, > > Maxim Cournoyer <maxim.cournoyer@gmail.com> writes: > >> +@cindex LGTM, Looks Good To Me >> +When you deem the proposed change adequate and ready for inclusion >> +within Guix, the following well understood/codified @acronym{LGTM, Looks >> +Good To Me} phrases should be used to sign off as a reviewer, meaning >> +you have reviewed the change and that it looks good to you: >> + >> +@itemize >> +@item >> +If the @emph{whole} series (containing multiple commits) looks good to >> +you, reply with a @samp{This series LGTM!} to the cover page if it has >> +one, or to the last patch of the series otherwise. >> + >> +@item >> +If you instead want to mark a @emph{single commit} as reviewed (but not >> +the whole series), simply reply with @samp{LGTM!} to that commit >> +message. >> +@end itemize > > Some mbox -> patches tools (like b4) are able to automatically apply > trailers like "Reviewed-by: some ident <...>" found as replies to patch > series [1]. I also like the very clear meaning of it: there's no way > you would type this without meaning "Yes, I officially vouch that this > patch series has been reviewed by me". Maybe this could be codified > here? Reading about it at <https://b4.docs.kernel.org/en/latest/maintainer/am-shazam.html>, it seems 'b4 -t' is about adding the 'Review-by' git trailers found in replies to *cover letters*, and inserting them on each commit of the series when applied. Do you use such a tool when applying patches working with Guix review? I'm not too fond of adding tool-specific bits to the otherwise tool-agnostic section above, but perhaps we could add a note or something.
Hi, These are a few questions regarding reviewing. 1. What should the reviewer do with old-style patches, like the ones that don't use G-Expressions? Should we tell the submitter to use them when possible or is it only a matter of style that is up to the submitter? Obviously they are hard to grasp for newcomers. It's probably good for newcomers if we teach them how to use G-Expressions but we don't really have time to do so, given the number of patches waiting to be reviewed. This question could be extended to style issues. Like using %var versus var. 2. What should the reviewer do when only small changes are required? The reviewer could do these changes in seconds whereas asking for a new revision could take days. These changes could be indentation fixes, removing of unused code, but they could also be more substantial, like adding a missing `file-name` field. Or changing old-style to G-Expressions? If the reviewer makes such changes and pushes them right away, I imagine they should be documented and explained. 3. Should the reviewer run the program being packaged? The above guidelines speak about applying, reading, building and linting but not running. (Making sure it works as expected.) Thanks, Clément
Hi, Clément Lassieur <clement@lassieur.org> writes: > Hi, > > These are a few questions regarding reviewing. > > 1. What should the reviewer do with old-style patches, like the ones > that don't use G-Expressions? Should we tell the submitter to use > them when possible or is it only a matter of style that is up to the > submitter? Obviously they are hard to grasp for newcomers. > > It's probably good for newcomers if we teach them how to use > G-Expressions but we don't really have time to do so, given the > number of patches waiting to be reviewed. > > This question could be extended to style issues. Like using %var > versus var. I think we should now make sure all new submissions use the current style; if they aren't we can demand of the contributors to adjust it. There is a blog post and enough examples in the code base already that should make this not too difficult. > 2. What should the reviewer do when only small changes are required? > The reviewer could do these changes in seconds whereas asking for a > new revision could take days. These changes could be indentation > fixes, removing of unused code, but they could also be more > substantial, like adding a missing `file-name` field. Or changing > old-style to G-Expressions? > > If the reviewer makes such changes and pushes them right away, I > imagine they should be documented and explained. In general, I think it's best to let the contributor know about the small problems so they can submit a v2, as they'll learn to pay attention to these. For first submissions, we can make the experience easier by adjusting locally and pushing directly for small things. This also holds for very old contributions where the original author is no longer around. I think these two points could be expound as new subsections of the 'Coding Style' section; the current scope is about codifying the human interactions more than the technical details. > 3. Should the reviewer run the program being packaged? The above > guidelines speak about applying, reading, building and linting but > not running. (Making sure it works as expected.) From the diff: --8<---------------cut here---------------start------------->8--- +Perhaps the biggest action you can do to help GNU Guix grow as a project +is to review the work contributed by others. You do not need to be a +committer to do so; applying, reading the source, building, linting and +running other people's series and sharing your comments about your +experience will give some confidence to committers, and should result in +the proposed change being merged faster. --8<---------------cut here---------------end--------------->8--- So it does mention trying out the software ("running").
Hi Maxim, Maxim Cournoyer <maxim.cournoyer@gmail.com> writes: >> 1. What should the reviewer do with old-style patches, like the ones >> that don't use G-Expressions? Should we tell the submitter to use >> them when possible or is it only a matter of style that is up to the >> submitter? Obviously they are hard to grasp for newcomers. >> >> It's probably good for newcomers if we teach them how to use >> G-Expressions but we don't really have time to do so, given the >> number of patches waiting to be reviewed. >> >> This question could be extended to style issues. Like using %var >> versus var. > > I think we should now make sure all new submissions use the current > style; if they aren't we can demand of the contributors to adjust it. > There is a blog post and enough examples in the code base already that > should make this not too difficult. Are you referring to this one? https://guix.gnu.org/en/blog/2023/dissecting-guix-part-3-g-expressions/ >> 2. What should the reviewer do when only small changes are required? >> The reviewer could do these changes in seconds whereas asking for a >> new revision could take days. These changes could be indentation >> fixes, removing of unused code, but they could also be more >> substantial, like adding a missing `file-name` field. Or changing >> old-style to G-Expressions? >> >> If the reviewer makes such changes and pushes them right away, I >> imagine they should be documented and explained. > > +Perhaps the biggest action you can do to help GNU Guix grow as a project > +is to review the work contributed by others. You do not need to be a > +committer to do so; applying, reading the source, building, linting and > +running other people's series and sharing your comments about your > +experience will give some confidence to committers, and should result in > +the proposed change being merged faster. > > So it does mention trying out the software ("running"). Yes indeed! I didn't see it. Thanks for your reply! Clément
Hi, Clément Lassieur <clement@lassieur.org> writes: > Hi Maxim, > > Maxim Cournoyer <maxim.cournoyer@gmail.com> writes: > >>> 1. What should the reviewer do with old-style patches, like the ones >>> that don't use G-Expressions? Should we tell the submitter to use >>> them when possible or is it only a matter of style that is up to the >>> submitter? Obviously they are hard to grasp for newcomers. >>> >>> It's probably good for newcomers if we teach them how to use >>> G-Expressions but we don't really have time to do so, given the >>> number of patches waiting to be reviewed. >>> >>> This question could be extended to style issues. Like using %var >>> versus var. >> >> I think we should now make sure all new submissions use the current >> style; if they aren't we can demand of the contributors to adjust it. >> There is a blog post and enough examples in the code base already that >> should make this not too difficult. > > Are you referring to this one? > https://guix.gnu.org/en/blog/2023/dissecting-guix-part-3-g-expressions/ Rather to the one corresponding to the 1.4.0 release, which introduced these new changes: <https://guix.gnu.org/en/blog/2022/gnu-guix-1.4.0-released/>.
Hi, On Fri, 20 Oct 2023 at 19:01, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote: >> 3. Should the reviewer run the program being packaged? The above >> guidelines speak about applying, reading, building and linting but >> not running. (Making sure it works as expected.) > --8<---------------cut here---------------start------------->8--- > +Perhaps the biggest action you can do to help GNU Guix grow as a project > +is to review the work contributed by others. You do not need to be a > +committer to do so; applying, reading the source, building, linting and > +running other people's series and sharing your comments about your > +experience will give some confidence to committers, and should result in > +the proposed change being merged faster. > --8<---------------cut here---------------end--------------->8--- > > So it does mention trying out the software ("running"). If LGTM also implies “I run it and it is OK”, then submitter should provide how to run it. Otherwise, for what it is worth, I will stop to review stuff that I do not use myself because reviewing is asking me too much: read some doc about how to run something that I do not care. ( Similarly, if LGTM also implies “I have read the source code and it is OK”, it appears to me too much. ) Well, “running” and “trying out the software” seems ambiguous. What does it mean “run IceCat” or “run Gmsh” or else? What I like with the proposal is that it makes better defined what are the expectations behind LGTM. But what means “running” is not clear for me. IMHO, it is worth to clearly state: 1. what helps the review process: « applying, reading the source, building, linting and running other people's series and sharing your comments about your experience will give some confidence to committers, and should result in the proposed change being merged faster. » 2. what means LGTM, from my understanding: applying, building, linting, carefully checking the code that is merged to Guix. Cheers, simon
Re, On Tue, 24 Oct 2023 at 10:49, Simon Tournier <zimon.toutoune@gmail.com> wrote: >> --8<---------------cut here---------------start------------->8--- >> +Perhaps the biggest action you can do to help GNU Guix grow as a project >> +is to review the work contributed by others. You do not need to be a >> +committer to do so; applying, reading the source, building, linting and >> +running other people's series and sharing your comments about your >> +experience will give some confidence to committers, and should result in >> +the proposed change being merged faster. >> --8<---------------cut here---------------end--------------->8--- [...] > IMHO, it is worth to clearly state: > > 1. what helps the review process: > > « applying, reading the source, building, linting and running other > people's series and sharing your comments about your experience will > give some confidence to committers, and should result in the > proposed change being merged faster. » Although I do not know if it is the right place, I would mention that “building” also means that the dependants still build or clearly indicate if something breaks. Cheers, simon
Hi, Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis: > * doc/contributing.texi (Contributing) [Reviewing the Work of Others]: New > section. > * doc/contributing.texi (Debbugs Usertags): Expound with Emacs Debbugs ^ Nitpick: no need to repeat the file name. > +@node Reviewing the Work of Others > +@section Reviewing the Work of Others [...] > +@cindex reviewing, guidelines > +Review comments should be unambiguous; be as clear and explicit as you > +can about what you think should be changed, ensuring the author can take > +action on it. Please try to keep the following guidelines in mind > +during review: [...] > +@item > +@emph{Remain focused: do not change the scope of the work being > +reviewed.} For example, if the contribution touches code that follows a > +pattern deemed unwieldy, it would be unfair to ask the submitter to fix > +all occurrences of that pattern in the code; to put it simply, if a > +problem unrelated to the patch at hand was already there, do not ask the > +submitter to fix it. Another item came to mind, that could be phrased like this: Spend time proportional to the stakes. Ensure the discussion focuses on important aspects of the changes; do not let it be derailed by objectively unimportant issues@footnote{This situation is often referred to as ``bikeshedding'', where much time is spent discussing each one's preference for the color of the shed at the expense progress made on the project to keep bikes dry.}. In particular, issues such as code formatting and other conventions can be dealt with through automation---e.g., @command{guix lint} and @command{guix style}---or through documentation (@pxref{Packaging Guidelines}, as an example). Maybe that makes the guidelines too long though; your call. Otherwise LGTM! Ludo’.
Hi On Tue, 24 Oct 2023 at 17:54, Ludovic Courtès <ludo@gnu.org> wrote: >> +@item >> +@emph{Remain focused: do not change the scope of the work being >> +reviewed.} For example, if the contribution touches code that follows a >> +pattern deemed unwieldy, it would be unfair to ask the submitter to fix >> +all occurrences of that pattern in the code; to put it simply, if a >> +problem unrelated to the patch at hand was already there, do not ask the >> +submitter to fix it. For me this item is clear… > Another item came to mind, that could be phrased like this: …while this new is unclear… > Spend time proportional to the stakes. Ensure the discussion focuses > on important aspects of the changes; do not let it be derailed by > objectively unimportant issues@footnote{This situation is often > referred to as ``bikeshedding'', where much time is spent discussing > each one's preference for the color of the shed at the expense > progress made on the project to keep bikes dry.}. In particular, > issues such as code formatting and other conventions can be dealt with > through automation---e.g., @command{guix lint} and @command{guix > style}---or through documentation (@pxref{Packaging Guidelines}, as an > example). …especially in the light of these other items: +@item +@emph{Review is a discussion.} The submitter's and reviewer's views on +how to achieve a particular change may not always be aligned. As a +reviewer, try hard to explain the rationale for suggestions you make, +and to understand and take into account the submitter's motivation for +doing things in a certain way. +@item +@emph{Aim for finalization.} Reviewing code is time-consuming. Your +goal as a reviewer is to put the process on a clear path towards +integration, possibly with agreed-upon changes, or rejection, with a +clear and mutually-understood reasoning. Avoid leaving the review +process in a lingering state with no clear way out. Well, I do not like: « discussion focuses on important aspects of the changes; do not let it be derailed by objectively unimportant issues » because it is not clear for me what means “important aspects” or “objectively unimportant issues”. How do I gauge? Sometimes, what does not appear to me “important” at first has then, at the end, an impact that cannot be neglected. This new item appears to me as: it is not a open discussion and you should refrain from commenting if you are not sure your point is *absolutely* important. Instead of this new item – where my understanding is: avoid bikeshed :-) and I agree – I propose to amend the item @emph{Review is a discussion.} by explicitly refer to the 3 other items; which are, IMHO, the guards against bikeshedding. Something along: --8<---------------cut here---------------start------------->8--- @item @emph{Review is a discussion.} The submitter's and reviewer's views on how to achieve a particular change may not always be aligned. The discussion is lead by remain focused, ensure progress and aim for finalization; spend time proportional to the stakes@footnote{This situation is often referred to as ``bikeshedding'', where much time is spent discussing each one's preference for the color of the shed at the expense progress made on the project to keep bikes dry.}. As a reviewer, try hard to explain the rationale for suggestions you make, and to understand and take into account the submitter's motivation for doing things in a certain way. --8<---------------cut here---------------end--------------->8--- WDYT? Does it capture the idea? If yes, I would pick this order for the enumeration: 1. Be clear and explicit about changes you are suggesting 2. Remain focused 3. Ensure progress 4. Aim for finalization 5. Review is a discussion Cheers, simon
Hi Maxim, Maxim Cournoyer <maxim.cournoyer@gmail.com> writes: > Do you use such a tool when applying patches working with Guix review? Yes, it's very useful. > I'm not too fond of adding tool-specific bits to the otherwise > tool-agnostic section above, but perhaps we could add a note or > something. You can view this as a tool-agnostic but clear way way of formally saying "I've reviewed this". Otherwise, it's not entirely clear when someone has reviewed a patch series IMO. Best,
Hi Simon, Simon Tournier <zimon.toutoune@gmail.com> writes: > Re, > > On Tue, 24 Oct 2023 at 10:49, Simon Tournier <zimon.toutoune@gmail.com> wrote: > >>> --8<---------------cut here---------------start------------->8--- >>> +Perhaps the biggest action you can do to help GNU Guix grow as a project >>> +is to review the work contributed by others. You do not need to be a >>> +committer to do so; applying, reading the source, building, linting and >>> +running other people's series and sharing your comments about your >>> +experience will give some confidence to committers, and should result in >>> +the proposed change being merged faster. >>> --8<---------------cut here---------------end--------------->8--- > > [...] > >> IMHO, it is worth to clearly state: >> >> 1. what helps the review process: >> >> « applying, reading the source, building, linting and running other >> people's series and sharing your comments about your experience will >> give some confidence to committers, and should result in the >> proposed change being merged faster. » > > Although I do not know if it is the right place, I would mention that > “building” also means that the dependants still build or clearly > indicate if something breaks. That's already mentioned in 'Submitting Patches': 9. Check that dependent packages (if applicable) are not affected by the change; ‘guix refresh --list-dependent PACKAGE’ will help you do that (*note Invoking guix refresh::). I don't think we should repeat it here :-) (also, we now have CI, which should be more apt at catching breakage here). -- Thanks, Maxim
Hi Maxim, On Tue, 31 Oct 2023 at 19:53, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote: > >> IMHO, it is worth to clearly state: [...] > That's already mentioned in 'Submitting Patches': [...] > I don't think we should repeat it here :-) (also, we now have CI, which > should be more apt at catching breakage here). We are listing the expectations for the Review process, therefore repeat that the dependencies need to be checked at Review time makes sense to me. It can be a sentence as: "make sure CI does not report new error" or something like that. Cheers, simon
diff --git a/doc/contributing.texi b/doc/contributing.texi index 864190b119..b8a0839c7f 100644 --- a/doc/contributing.texi +++ b/doc/contributing.texi @@ -29,6 +29,7 @@ Contributing * Submitting Patches:: Share your work. * Tracking Bugs and Changes:: Keeping it all organized. * Commit Access:: Pushing to the official repository. +* Reviewing the Work of Others:: Some guidelines for sharing reviews. * Updating the Guix Package:: Updating the Guix package definition. * Writing Documentation:: Improving documentation in GNU Guix. * Translating Guix:: Make Guix speak your native language. @@ -605,7 +606,7 @@ Packaging Guidelines * Version Numbers:: When the name is not enough. * Synopses and Descriptions:: Helping users find the right package. * Snippets versus Phases:: Whether to use a snippet, or a build phase. -* Cyclic Module Dependencies:: Going full circle. +* Cyclic Module Dependencies:: Going full circle. * Emacs Packages:: Your Elisp fix. * Python Modules:: A touch of British comedy. * Perl Modules:: Little pearls. @@ -1972,7 +1973,12 @@ Debbugs Usertags tag any bug with an arbitrary label. Bugs can be searched by usertag, so this is a handy way to organize bugs@footnote{The list of usertags is public information, and anyone can modify any user's list of usertags, -so keep that in mind if you choose to use this feature.}. +so keep that in mind if you choose to use this feature.}. If you use +Emacs Debbugs, the entry-point to consult existing usertags is the +@samp{C-u M-x debbugs-gnu-usertags} procedure. To set a usertag, press +@samp{C} while consulting a bug within the *Guix-Patches* buffer opened +with @samp{C-u M-x debbugs-gnu-bugs} buffer, then select @code{usertag} +and follow the instructions. For example, to view all the bug reports (or patches, in the case of @code{guix-patches}) tagged with the usertag @code{powerpc64le-linux} @@ -1985,9 +1991,9 @@ Debbugs Usertags to interact with Debbugs. In Guix, we are experimenting with usertags to keep track of -architecture-specific issues. To facilitate collaboration, all our -usertags are associated with the single user @code{guix}. The following -usertags currently exist for that user: +architecture-specific issues, as well as reviewed ones. To facilitate +collaboration, all our usertags are associated with the single user +@code{guix}. The following usertags currently exist for that user: @table @code @@ -2005,6 +2011,9 @@ Debbugs Usertags appropriate to assign this usertag to a bug report for a package that fails to build reproducibly. +@item reviewed-looks-good +You have reviewed the series and it looks good to you (LGTM). + @end table If you're a committer and you want to add a usertag, just start using it @@ -2237,6 +2246,80 @@ Commit Access you're welcome to use your expertise and commit rights to help other contributors, too! +@node Reviewing the Work of Others +@section Reviewing the Work of Others + +Perhaps the biggest action you can do to help GNU Guix grow as a project +is to review the work contributed by others. You do not need to be a +committer to do so; applying, reading the source, building, linting and +running other people's series and sharing your comments about your +experience will give some confidence to committers, and should result in +the proposed change being merged faster. + +@cindex reviewing, guidelines +Review comments should be unambiguous; be as clear and explicit as you +can about what you think should be changed, ensuring the author can take +action on it. Please try to keep the following guidelines in mind +during review: + +@enumerate +@item +@emph{Be clear and explicit about changes you are suggesting}, ensuring +the author can take action on it. In particular, it is a good idea to +explicitly ask for new revisions when you want it. + +@item +@emph{Remain focused: do not change the scope of the work being +reviewed.} For example, if the contribution touches code that follows a +pattern deemed unwieldy, it would be unfair to ask the submitter to fix +all occurrences of that pattern in the code; to put it simply, if a +problem unrelated to the patch at hand was already there, do not ask the +submitter to fix it. + +@item +@emph{Ensure progress.} As they respond to review, submitters may +submit new revisions of their changes; avoid requesting changes that you +did not request in the previous round of comments. Overall, the +submitter should get a clear sense of progress; the number of items open +for discussion should clearly decrease over time. + +@item +@emph{Review is a discussion.} The submitter's and reviewer's views on +how to achieve a particular change may not always be aligned. As a +reviewer, try hard to explain the rationale for suggestions you make, +and to understand and take into account the submitter's motivation for +doing things in a certain way. + +@item +@emph{Aim for finalization.} Reviewing code is time-consuming. Your +goal as a reviewer is to put the process on a clear path towards +integration, possibly with agreed-upon changes, or rejection, with a +clear and mutually-understood reasoning. Avoid leaving the review +process in a lingering state with no clear way out. +@end enumerate + +@cindex LGTM, Looks Good To Me +When you deem the proposed change adequate and ready for inclusion +within Guix, the following well understood/codified @acronym{LGTM, Looks +Good To Me} phrases should be used to sign off as a reviewer, meaning +you have reviewed the change and that it looks good to you: + +@itemize +@item +If the @emph{whole} series (containing multiple commits) looks good to +you, reply with a @samp{This series LGTM!} to the cover page if it has +one, or to the last patch of the series otherwise. + +@item +If you instead want to mark a @emph{single commit} as reviewed (but not +the whole series), simply reply with @samp{LGTM!} to that commit +message. +@end itemize + +If you are not a committer, you can help others find a @emph{series} you +have reviewed more easily by adding a @code{reviewed-looks-good} usertag +for the @code{guix} user (@pxref{Debbugs Usertags}). + @node Updating the Guix Package @section Updating the Guix Package