Re: [PATCH wayland 1/2] contributing: add review guidelines
Hi Pekka, On 18 June 2018 at 14:42, Pekka Paalanen wrote: > +- Stable ABI or API is not broken. > + I think I've just caught one of those ;-) Thanks for the vast, yet concise writeup. Fwiw Reviewed-by: Emil Velikov -Emil ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland 1/2] contributing: add review guidelines
On Tue, 19 Jun 2018 11:45:24 +0100 Daniel Stone wrote: > Hi Pekka, > > On Mon, 18 Jun 2018 at 14:43, Pekka Paalanen wrote: > > This sets up the standards for patch review, and defines when a patch > > can be merged. I believe these are the practises we have been using > > already for a long time, now they are just written down explicitly. > > > > It's not an exhaustive list of criteria and likely cannot ever be, but > > it should give a good idea of what level of review we want to have. > > > > It has been written in general terms, so that we can easily apply the > > same text not just to Wayland, but also Weston and other projects as > > necessary. > > > > This addition is not redundant with > > https://wayland.freedesktop.org/reviewing.html . > > > > The web page is a friendly introduction and encouragement for people to > > get involved. The guidelines here are more specific and aimed for people > > who seek commit rights or maintainership. > > Thanks a lot for tackling this! Having it written down is fantastic, > and I like how you've put it. I have two minor suggestions below: > > > +- The code is correct and does not ignore corner-cases. > > In general this is what we aim for, but the presence of 'XXX'/'FIXME' > comments in the code shows we don't always live up to that ideal. ;) > Maybe this could instead be something like: > > The code is correct and does not introduce new failures for existing > users, nor add new corner-case bugs. Yes, that was my intention. > When fixing bugs, 'root cause' fixes are preferred rather than hiding > symptoms: identify why the issue has occurred, and fix it as close as > possible to the source. If it is not practical to completely fix the > issue, partial fixes should be accompanied by a comment in proximate > code, as well as a new issue opened in GitLab clearly noting known > corner cases with a suggestion on how to fix them. A good point. > > > +- The code does what it says in the commit message and changes nothing > > else. > > Which leads naturally to something like: > > If the commit message starts getting too long and addresses multiple > points, this may be a sign that the commit should be broken into > smaller individual commits. Yes. > I'm happy to take alternate wording, since what you've written here is > far more concise than mine. Anyway, both patches are: > Reviewed-by: Daniel Stone On one hand I'd like it to be very concise, so that people don't get put off by it. The list already seemed a bit long, and one could write books about it. On the other hand I'd like to expand on all points. Should we favour short bullet points or longer explanations? The Markdown formatting also sets some restrictions, and I think it's likely the list will only grow longer. I suppose your intention is that we can land these two patches as is, and make amendments in follow-ups, right? I'd like to let this soak on the mailing list for a while and see what other comments and R-bs we get. > Could this later be used as the basis for similar Weston patches? I assumed we just point here from Weston, if we don't already, at least until differences between Wayland and Weston emerge. Thanks, pq pgpyRkZ2mMMuL.pgp Description: OpenPGP digital signature ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland 1/2] contributing: add review guidelines
Hi Pekka, On Mon, 18 Jun 2018 at 14:43, Pekka Paalanen wrote: > This sets up the standards for patch review, and defines when a patch > can be merged. I believe these are the practises we have been using > already for a long time, now they are just written down explicitly. > > It's not an exhaustive list of criteria and likely cannot ever be, but > it should give a good idea of what level of review we want to have. > > It has been written in general terms, so that we can easily apply the > same text not just to Wayland, but also Weston and other projects as > necessary. > > This addition is not redundant with > https://wayland.freedesktop.org/reviewing.html . > > The web page is a friendly introduction and encouragement for people to > get involved. The guidelines here are more specific and aimed for people > who seek commit rights or maintainership. Thanks a lot for tackling this! Having it written down is fantastic, and I like how you've put it. I have two minor suggestions below: > +- The code is correct and does not ignore corner-cases. In general this is what we aim for, but the presence of 'XXX'/'FIXME' comments in the code shows we don't always live up to that ideal. ;) Maybe this could instead be something like: The code is correct and does not introduce new failures for existing users, nor add new corner-case bugs. When fixing bugs, 'root cause' fixes are preferred rather than hiding symptoms: identify why the issue has occurred, and fix it as close as possible to the source. If it is not practical to completely fix the issue, partial fixes should be accompanied by a comment in proximate code, as well as a new issue opened in GitLab clearly noting known corner cases with a suggestion on how to fix them. > +- The code does what it says in the commit message and changes nothing else. Which leads naturally to something like: If the commit message starts getting too long and addresses multiple points, this may be a sign that the commit should be broken into smaller individual commits. I'm happy to take alternate wording, since what you've written here is far more concise than mine. Anyway, both patches are: Reviewed-by: Daniel Stone Could this later be used as the basis for similar Weston patches? Cheers, Daniel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland 1/2] contributing: add review guidelines
Reviewed-by: Matheus Santana It seems a great starting point. On Mon, Jun 18, 2018 at 10:42 AM, Pekka Paalanen wrote: > From: Pekka Paalanen > > This sets up the standards for patch review, and defines when a patch > can be merged. I believe these are the practises we have been using > already for a long time, now they are just written down explicitly. > > It's not an exhaustive list of criteria and likely cannot ever be, but > it should give a good idea of what level of review we want to have. > > It has been written in general terms, so that we can easily apply the > same text not just to Wayland, but also Weston and other projects as > necessary. > > This addition is not redundant with > https://wayland.freedesktop.org/reviewing.html . > > The web page is a friendly introduction and encouragement for people to > get involved. The guidelines here are more specific and aimed for people > who seek commit rights or maintainership. > > Signed-off-by: Pekka Paalanen > --- > CONTRIBUTING.md | 49 + > 1 file changed, 49 insertions(+) > > diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md > index 7ad75b8..6e74b6d 100644 > --- a/CONTRIBUTING.md > +++ b/CONTRIBUTING.md > @@ -197,5 +197,54 @@ New source code files should specify the MIT Expat > license in their > boilerplate, as part of the copyright statement. > > > +Review > +== > + > +All patches, even trivial ones, require at least one positive review > +(Reviewed-by). Additionally, if no Reviewed-by's have been given by > +people with commit access, there needs to be at least one Acked-by from > +someone with commit access. A person with commit access is expected to be > +able to evaluate the patch with respect to the project scope and > architecture. > + > +During review, the following matters should be checked: > + > +- The commit message explains why the change is being made. > + > +- The code fits the project's scope. > + > +- The code license is the same MIT licence the project generally uses. > + > +- Stable ABI or API is not broken. > + > +- Stable ABI or API additions must be justified by actual use cases, not > only > +by speculation. They must also be documented, and it is strongly > recommended to > +include tests excercising the additions in the test suite. > + > +- The code fits the existing software architecture, e.g. no layering > +violations. > + > +- The code is correct and does not ignore corner-cases. > + > +- In a patch series, every intermediate step produces correct code as > well. > + > +- The code does what it says in the commit message and changes nothing > else. > + > +- The test suite passes. > + > +- The code does not depend on API or ABI which has no working free open > source > +implementation. > + > +- The code is not dead or untestable. E.g. if there are no free open > source > +software users for it then it is effectively dead code. > + > +- The code is written to be easy to understand, or if code cannot be clear > +enough on its own there are code comments to explain it. > + > +- The code is minimal, i.e. prefer refactor and re-use when possible > unless > +clarity suffers. > + > +- The code adheres to the style guidelines. > + > + > [git documentation]: http://git-scm.com/documentation > [notes on commit messages]: http://who-t.blogspot.de/2009/ > 12/on-commit-messages.html > -- > 2.16.4 > > ___ > wayland-devel mailing list > wayland-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/wayland-devel > ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH wayland 1/2] contributing: add review guidelines
From: Pekka Paalanen This sets up the standards for patch review, and defines when a patch can be merged. I believe these are the practises we have been using already for a long time, now they are just written down explicitly. It's not an exhaustive list of criteria and likely cannot ever be, but it should give a good idea of what level of review we want to have. It has been written in general terms, so that we can easily apply the same text not just to Wayland, but also Weston and other projects as necessary. This addition is not redundant with https://wayland.freedesktop.org/reviewing.html . The web page is a friendly introduction and encouragement for people to get involved. The guidelines here are more specific and aimed for people who seek commit rights or maintainership. Signed-off-by: Pekka Paalanen --- CONTRIBUTING.md | 49 + 1 file changed, 49 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7ad75b8..6e74b6d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -197,5 +197,54 @@ New source code files should specify the MIT Expat license in their boilerplate, as part of the copyright statement. +Review +== + +All patches, even trivial ones, require at least one positive review +(Reviewed-by). Additionally, if no Reviewed-by's have been given by +people with commit access, there needs to be at least one Acked-by from +someone with commit access. A person with commit access is expected to be +able to evaluate the patch with respect to the project scope and architecture. + +During review, the following matters should be checked: + +- The commit message explains why the change is being made. + +- The code fits the project's scope. + +- The code license is the same MIT licence the project generally uses. + +- Stable ABI or API is not broken. + +- Stable ABI or API additions must be justified by actual use cases, not only +by speculation. They must also be documented, and it is strongly recommended to +include tests excercising the additions in the test suite. + +- The code fits the existing software architecture, e.g. no layering +violations. + +- The code is correct and does not ignore corner-cases. + +- In a patch series, every intermediate step produces correct code as well. + +- The code does what it says in the commit message and changes nothing else. + +- The test suite passes. + +- The code does not depend on API or ABI which has no working free open source +implementation. + +- The code is not dead or untestable. E.g. if there are no free open source +software users for it then it is effectively dead code. + +- The code is written to be easy to understand, or if code cannot be clear +enough on its own there are code comments to explain it. + +- The code is minimal, i.e. prefer refactor and re-use when possible unless +clarity suffers. + +- The code adheres to the style guidelines. + + [git documentation]: http://git-scm.com/documentation [notes on commit messages]: http://who-t.blogspot.de/2009/12/on-commit-messages.html -- 2.16.4 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
