Re: [Development] [FYI] commit/review policy refactored
On Wed, Jul 13, 2016 at 12:15:44PM +0200, Kai Köhne wrote:
> The page is obviously written from the viewpoint of a maintainer. I'd
> prefer to write It in a less intimidating way to the contributor, and
> make it helpful for first time, inexperienced contributors.
>
first-timers can ask in case of doubt. this is meant to be a policy, not
an intro. somewhat unsurprisingly, i finally took the effort to do this
work because i witnessed several unrelated violations in the last weeks/
months.
> > As a Contributor
>
> Maybe link back to https://wiki.qt.io/Commit_Policy .
>
it's linked from the reviewer section, which i find more logical.
> "Make sure that your commit matches the Qt Commit Policy.
>
> > 1. Invite relevant reviewers.
> > * Always invite the respective domain experts, not somebody convenient.
>
> Scrap the 'not somebody convenient'. It's the job of the reviewer to decide
> what he can approve.
>
it's the responsibility of both sides. don't pretend that these social
dynamics don't exist.
> Instead, mention how one can find the 'domain expert'. Something like
>
> * Domain experts can usually be found by inspecting the git log, and mailing
> lists. If in doubt also add the [https://wiki.qt.io/Maintainers Maintainer]
> of an area if there is one.
>
that's already listed in the contribution guidelines and i wanted to
avoid excessive redundancy. i may reconsider, or move parts of the
content. some more linking is necessary anyway.
> > 2. Give reviewers ample time to respond.
> > * Unless everyone who can be reasonably expected to have a relevant opinion
> > to offer has already done so, a full working day waiting time is the
> > absolute minimum; three days are reasonable.
> > * In particular, give watchers (usually higher-level maintainers) enough
> > time to voice concerns even if you did not explicitly invite them.
>
> The sub-points are only valid if you have a +2 already. So maybe move them
> down to a section ("If your change got approved"). Rather mention that it can
> take some working days until added people respond.
>
that's a good idea. a section about staging and re-staging is needed
anyway.
> > 3. Discuss objections. Do not override a -1 unless there is a broad expert
> > consensus that the objection is unfounded.
>
> A Contributor cannot usually 'override' a -1 - that can only be done by an
> approver. Maybe your point is though that, if you got a +2, and somebody
> voiced objections, one shouldn't stage it?
>
every approver is a contributor by definition. ;)
but i guess "disregard" is a better word than "override" in this case.
> > 4. Do not ignore/fight the Early Warning System. Justify each override.
> > [...]
>
> Isn't that limited to Approvers, too?
>
...
> > 5. Do not approve your own changes.
> [...]
>
> Again that's limited to Approvers,
>
yes
> and should be in the 'As a Reviewer' section.
>
no. it's addressing the contributor.
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] [FYI] commit/review policy refactored
> -Original Message-
> From: Development [mailto:development-bounces+kai.koehne=qt.io@qt-
> project.org] On Behalf Of Oswald Buddenhagen
> Sent: Wednesday, July 13, 2016 11:08 AM
> To: development@qt-project.org; qt-crea...@qt-project.org
> Subject: [Development] [FYI] commit/review policy refactored
>
> just a heads-up that i (finally) split off the review policy from the commit
> policy. see https://wiki.qt.io/Review_Policy .
> if you have suggestions for improvement, please discuss them upfront on irc.
I prefer to reply here :)
The page is obviously written from the viewpoint of a maintainer. I'd prefer to
write
It in a less intimidating way to the contributor, and make it helpful for first
time,
inexperienced contributors.
> As a Contributor
Maybe link back to https://wiki.qt.io/Commit_Policy .
"Make sure that your commit matches the Qt Commit Policy.
> 1. Invite relevant reviewers.
> * Always invite the respective domain experts, not somebody convenient.
Scrap the 'not somebody convenient'. It's the job of the reviewer to decide
what he can approve.
Instead, mention how one can find the 'domain expert'. Something like
* Domain experts can usually be found by inspecting the git log, and mailing
lists. If in doubt also add the [https://wiki.qt.io/Maintainers Maintainer] of
an area if there is one.
> 2. Give reviewers ample time to respond.
> * Unless everyone who can be reasonably expected to have a relevant opinion
> to offer has already done so, a full working day waiting time is the absolute
> minimum; three days are reasonable.
> * In particular, give watchers (usually higher-level maintainers) enough time
> to voice concerns even if you did not explicitly invite them.
The sub-points are only valid if you have a +2 already. So maybe move them down
to a section ("If your change got approved"). Rather mention that it can take
some working days until added people respond.
> 3. Discuss objections. Do not override a -1 unless there is a broad expert
> consensus that the objection is unfounded.
A Contributor cannot usually 'override' a -1 - that can only be done by an
approver. Maybe your point is though that, if you got a +2, and somebody voiced
objections, one shouldn't stage it?
> 4. Do not ignore/fight the Early Warning System. Justify each override.
> [...]
Isn't that limited to Approvers, too?
> 5. Do not approve your own changes.
[...]
Again that's limited to Approvers, and should be in the 'As a Reviewer'
section.
Regards
Kai
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] [FYI] commit/review policy refactored
On Wed, Jul 13, 2016 at 11:57:28AM +0200, Martin Smith wrote: > Sometimes I am listed as a reviewer for a code change in Qt that also involves > documentation changes. The code change is sometimes outside my expertise, but > I > assume I have been included to approve the documentation changes. I always > give > +1 in such cases. Is that correct? > yes. i added a respective point to the policy. ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] [FYI] commit/review policy refactored
Sometimes I am listed as a reviewer for a code change in Qt that also involves documentation changes. The code change is sometimes outside my expertise, but I assume I have been included to approve the documentation changes. I always give +1 in such cases. Is that correct? martin From: Development on behalf of Oswald Buddenhagen Sent: Wednesday, July 13, 2016 11:08:07 AM To: development@qt-project.org; qt-crea...@qt-project.org Subject: [Development] [FYI] commit/review policy refactored just a heads-up that i (finally) split off the review policy from the commit policy. see https://wiki.qt.io/Review_Policy . if you have suggestions for improvement, please discuss them upfront on irc. you're expected to read the document even if you're not interested in actively improving it any time soon. ;) ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
