[PATCH wayland] contributing: review rules for bugs

[Pekka Paalanen]

On Wed, 27 Jun 2018 17:49:34 +0100
Emil Velikov <emil.l.velikov at gmail.com> wrote:

> Hi Pekka,
> 
> A couple small ideas come to mind:
> 
> On 27 June 2018 at 14:47, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> >
> > Half of the ideas came from Daniel but most of them are reworded, the
> > rest are my thoughts.
> >
> > Mention compiler warnings specifically, and be more explicit on what
> > kind of code or bugs or bug fixes are acceptable or not. Clarify commit
> > scope.
> >
> > Cc: Daniel Stone <daniels at collabora.com>
> > Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > ---
> >  CONTRIBUTING.md | 19 ++++++++++++++++---
> >  1 file changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
> > index 70d0eca..51bef89 100644
> > --- a/CONTRIBUTING.md
> > +++ b/CONTRIBUTING.md
> > @@ -223,11 +223,24 @@ 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.
> > +- The code is correct and does not introduce new failures for existing users,
> > +does not add new corner-case bugs, and does not introduce new compiler
> > +warnings.  
> Compiler warnings vary greatly on the compiler and it's version. It
> does become a nuisance when committer/reviewer is using a newer
> version which flags dozens of warnings.

Yes. There is no need to get out of one's way to hunt for possible
warnings, but if someone does notice new warnings, they should be
fixed. If a reviewer gets them but the author/submitter doesn't, the
reviewer can help with them. It's common sense to me.

I've been wondering if the CI build should actually turn -Werror on,
but that's better to discuss after if/when we migrate to MR-based work
flow.

> That aside, worth loosening this for patches where existing code is
> copied or moved?

It is already loosened: they are not new warnings if they were there
before. If code motion results in new warnings, they should ideally be
fixed. If the fix makes the code motion harder to review, it's ok to
fix it in another patch before or after.

I think we also need to be picky on how much details we are going to
write down here. It is very easy to extend the guidelines with details
so far that it becomes too long to read. That is why I try to be as
terse as possible.

It's the spirit of the guidelines that matters, they are not to be
interpreted by the letter, and there may always be special
circumstances where the reasonable action is to overlook something.
Even with the guidelines, we still rely very much on the individual
judgement. The important thing is to get people to the right mindset in
general.

Perhaps the above would be worth a paragraph in CONTRUBUTING.md?

> >
> > -- In a patch series, every intermediate step produces correct code as well.
> > +- In a patch series, every intermediate step produces correct and warning-free
> > +code as well.
> >  
> It makes sense to move this as the final bullet-point and let it refer
> to the whole section.
> Namely:
> 
> - In a patch series, every intermediate step adheres to the guidelines above.

Yes, a good suggestion.


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/20180628/517bf9bd/attachment.sig>