[PATCH wayland 1/2] contributing: add review guidelines

[Pekka Paalanen]

On Tue, 19 Jun 2018 11:45:24 +0100
Daniel Stone <daniel at fooishbar.org> wrote:

> Hi Pekka,
> 
> On Mon, 18 Jun 2018 at 14:43, Pekka Paalanen <ppaalanen at gmail.com> 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 <daniels at collabora.com>

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180619/6fa3025d/attachment.sig>