Super-review, what shall we do with you?

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

Super-review, what shall we do with you?

Dave Townsend
We've had a policy requiring super-review for certain kinds of patches
for a long time. It's changed a couple of times but the current policy
(http://www.mozilla.org/hacking/reviewers.html) primarily requires
super-review for any patch that introduces or changes an API. Basically
any function in a JS file or JSM is covered here, or at least that is my
reading of it. The reasoning is pretty straightforward, designing APIs
well up front reduces the maintenance burden in the future and hopefully
means we don't have to make changes that break add-ons.

The problem is that I frequently come across patches that were landed
without super-review despite meeting the requirements. This suggests
that many reviewers aren't aware of the policy or don't have the same
interpretation of it as I do. We probably need to do a better job of
making sure that all reviewers in particular understand the policy and
are following it.

But, I haven't yet seen an issue arise from this lack of SR. Does that
mean that the policy is too restrictive and we need to change it to more
closely match how reviewers are working?

Either we're setting ourselves up for big problems down the road or we
have a policy that is in some cases hindering development. Those are the
extremes of course and the reality is probably somewhere in between, but
I'd like to hear other people's thoughts about this.
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Super-review, what shall we do with you?

Dave Townsend
On 11/06/12 10:09, Dave Townsend wrote:

> We've had a policy requiring super-review for certain kinds of patches
> for a long time. It's changed a couple of times but the current policy
> (http://www.mozilla.org/hacking/reviewers.html) primarily requires
> super-review for any patch that introduces or changes an API. Basically
> any function in a JS file or JSM is covered here, or at least that is my
> reading of it. The reasoning is pretty straightforward, designing APIs
> well up front reduces the maintenance burden in the future and hopefully
> means we don't have to make changes that break add-ons.
>
> The problem is that I frequently come across patches that were landed
> without super-review despite meeting the requirements. This suggests
> that many reviewers aren't aware of the policy or don't have the same
> interpretation of it as I do. We probably need to do a better job of
> making sure that all reviewers in particular understand the policy and
> are following it.
>
> But, I haven't yet seen an issue arise from this lack of SR. Does that
> mean that the policy is too restrictive and we need to change it to more
> closely match how reviewers are working?
>
> Either we're setting ourselves up for big problems down the road or we
> have a policy that is in some cases hindering development. Those are the
> extremes of course and the reality is probably somewhere in between, but
> I'd like to hear other people's thoughts about this.

Note, please post replies in mozilla.dev.platform.

_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Super-review, what shall we do with you?

Dave Townsend
In reply to this post by Dave Townsend
On 11/06/12 10:09, Dave Townsend wrote:

> We've had a policy requiring super-review for certain kinds of patches
> for a long time. It's changed a couple of times but the current policy
> (http://www.mozilla.org/hacking/reviewers.html) primarily requires
> super-review for any patch that introduces or changes an API. Basically
> any function in a JS file or JSM is covered here, or at least that is my
> reading of it. The reasoning is pretty straightforward, designing APIs
> well up front reduces the maintenance burden in the future and hopefully
> means we don't have to make changes that break add-ons.
>
> The problem is that I frequently come across patches that were landed
> without super-review despite meeting the requirements. This suggests
> that many reviewers aren't aware of the policy or don't have the same
> interpretation of it as I do. We probably need to do a better job of
> making sure that all reviewers in particular understand the policy and
> are following it.
>
> But, I haven't yet seen an issue arise from this lack of SR. Does that
> mean that the policy is too restrictive and we need to change it to more
> closely match how reviewers are working?
>
> Either we're setting ourselves up for big problems down the road or we
> have a policy that is in some cases hindering development. Those are the
> extremes of course and the reality is probably somewhere in between, but
> I'd like to hear other people's thoughts about this.

I've made a blog post suggesting a change to the definition of what an
API is that requires super-review. I'd appreciate it if people read it
and gave me some feedback:
http://www.oxymoronical.com/blog/2012/11/What-is-an-API
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Super-review, what shall we do with you?

Ehsan Akhgari
On 2012-11-23 2:29 PM, Dave Townsend wrote:
> I've made a blog post suggesting a change to the definition of what an
> API is that requires super-review. I'd appreciate it if people read it
> and gave me some feedback:
> http://www.oxymoronical.com/blog/2012/11/What-is-an-API

I don't think it makes a lot of sense to require super-review for
patches which implement web facing features which are spec'ed, since in
those cases we're really implementing APIs defined by the spec.


Speaking in a bit broader sense, I'm not really convinced that there is
actually a problem to be solved in the way that we currently do things
(defer whether or not a super-review is required to the author's and
reviewer's best judgement).  And requiring super-review on every patch
which modifies an API in the sense that is defined in your blog post
will mean that a ton of the patches which are currently being landed
will require a round of super-reviews, which means more review requests
to the list of super-reviewers (which has been pretty much static for
the past few years) and slower turn-around times...

Cheers,
Ehsan
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Super-review, what shall we do with you?

Alexander Surkov
In reply to this post by Dave Townsend
It doesn't make an exception for specific areas like accessibility. It
seems accessibility module never had super reviewers in peers and it
seems we never followed the rule "super review for API change". It
sounds like this rule adds unnecessary complexity for this module.

Thank you.
Alexander.


On Sat, Nov 24, 2012 at 4:29 AM, Dave Townsend <[hidden email]> wrote:
> I've made a blog post suggesting a change to the definition of what an API
> is that requires super-review. I'd appreciate it if people read it and gave
> me some feedback: http://www.oxymoronical.com/blog/2012/11/What-is-an-API
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Super-review, what shall we do with you?

Mook-6
In reply to this post by Dave Townsend
On 11/23/2012 3:42 PM, Ehsan Akhgari wrote:
> On 2012-11-23 2:29 PM, Dave Townsend wrote:
>> I've made a blog post suggesting a change to the definition of what an
>> API is that requires super-review. I'd appreciate it if people read it
>> and gave me some feedback:
>> http://www.oxymoronical.com/blog/2012/11/What-is-an-API
>
<snip>

>
> Speaking in a bit broader sense, I'm not really convinced that there is
> actually a problem to be solved in the way that we currently do things
> (defer whether or not a super-review is required to the author's and
> reviewer's best judgement).  And requiring super-review on every patch
> which modifies an API in the sense that is defined in your blog post
> will mean that a ton of the patches which are currently being landed
> will require a round of super-reviews, which means more review requests
> to the list of super-reviewers (which has been pretty much static for
> the past few years) and slower turn-around times...
>

Sounds like it would be useful to get more super-reviewers on board and
get faster turnaround times, then.  Or switch API review to a superset
of SRs, if it's really just there to get a second set of eyes on it.
Given the, umm, interesting quality of some of the new-to-me code, it
seems like having better oversight of at least the inter-module bits
would be helpful.  See also the thread from September about comments - I
honestly would have expected SRs to catch that.  (The interface given as
an example in that message has no comments, and no sr= marking of any sort.)

--
Mook
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Super-review, what shall we do with you?

Kyle Huey-2
In reply to this post by Ehsan Akhgari
On Nov 23, 2012 3:43 PM, "Ehsan Akhgari" <[hidden email]> wrote:

>
> On 2012-11-23 2:29 PM, Dave Townsend wrote:
>>
>> I've made a blog post suggesting a change to the definition of what an
>> API is that requires super-review. I'd appreciate it if people read it
>> and gave me some feedback:
>> http://www.oxymoronical.com/blog/2012/11/What-is-an-API
>
>
> I don't think it makes a lot of sense to require super-review for patches
which implement web facing features which are spec'ed, since in those cases
we're really implementing APIs defined by the spec.

I won't go so far as to claim that superreview is the best way to do this,
but web specs do need careful API review before we implement them.  The sr
requirement does help there.

> Speaking in a bit broader sense, I'm not really convinced that there is
actually a problem to be solved in the way that we currently do things
(defer whether or not a super-review is required to the author's and
reviewer's best judgement).  And requiring super-review on every patch
which modifies an API in the sense that is defined in your blog post will
mean that a ton of the patches which are currently being landed will
require a round of super-reviews, which means more review requests to the
list of super-reviewers (which has been pretty much static for the past few
years) and slower turn-around times...
>
> Cheers,
> Ehsan
>
> _______________________________________________
> dev-planning mailing list
> [hidden email]
> https://lists.mozilla.org/listinfo/dev-planning

- Kyle
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Super-review, what shall we do with you?

Mounir Lamouri-3
In reply to this post by Ehsan Akhgari
On 23/11/12 23:42, Ehsan Akhgari wrote:
> I don't think it makes a lot of sense to require super-review for
> patches which implement web facing features which are spec'ed, since in
> those cases we're really implementing APIs defined by the spec.

Blindly implementing Web API isn't a good practice. We very often
propose changes to specifications because we found issues while
implementing.

In practice, I think most DOM peers are super-reviewers which means that
most of the time I don't really bother asking a super-review unless I
really feel like we need double-checking the change.

However, I think super-reviews a generally a good tool. With the Firefox
OS effort, it allowed us to clearly differentiate the API review with
the implementation reviews.

Cheers,
--
Mounir
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Super-review, what shall we do with you?

Dave Townsend
In reply to this post by Dave Townsend
On 11/23/12 16:52, Alexander Surkov wrote:
> It doesn't make an exception for specific areas like accessibility. It
> seems accessibility module never had super reviewers in peers and it
> seems we never followed the rule "super review for API change". It
> sounds like this rule adds unnecessary complexity for this module.

I'm really only talking about the API review part of the super-review
rules here. The listed exceptions would still apply. Interestingly
though accessibility isn't currently listed as an exception to the
super-review rules.

_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning
Reply | Threaded
Open this post in threaded view
|

Re: Super-review, what shall we do with you?

Dave Townsend
In reply to this post by Dave Townsend
On 11/23/12 11:29, Dave Townsend wrote:

> On 11/06/12 10:09, Dave Townsend wrote:
>> We've had a policy requiring super-review for certain kinds of patches
>> for a long time. It's changed a couple of times but the current policy
>> (http://www.mozilla.org/hacking/reviewers.html) primarily requires
>> super-review for any patch that introduces or changes an API. Basically
>> any function in a JS file or JSM is covered here, or at least that is my
>> reading of it. The reasoning is pretty straightforward, designing APIs
>> well up front reduces the maintenance burden in the future and hopefully
>> means we don't have to make changes that break add-ons.
>>
>> The problem is that I frequently come across patches that were landed
>> without super-review despite meeting the requirements. This suggests
>> that many reviewers aren't aware of the policy or don't have the same
>> interpretation of it as I do. We probably need to do a better job of
>> making sure that all reviewers in particular understand the policy and
>> are following it.
>>
>> But, I haven't yet seen an issue arise from this lack of SR. Does that
>> mean that the policy is too restrictive and we need to change it to more
>> closely match how reviewers are working?
>>
>> Either we're setting ourselves up for big problems down the road or we
>> have a policy that is in some cases hindering development. Those are the
>> extremes of course and the reality is probably somewhere in between, but
>> I'd like to hear other people's thoughts about this.
>
> I've made a blog post suggesting a change to the definition of what an
> API is that requires super-review. I'd appreciate it if people read it
> and gave me some feedback:
> http://www.oxymoronical.com/blog/2012/11/What-is-an-API

It's a bit difficult to draw a consensus from everyone here but mostly
it seems that people think the status quo with module owners and
reviewers making their judgement is working out here. I'm going to email
all the module owners directly and ask that they look over the policy
and communicate with their peers about what their expectations are.
_______________________________________________
dev-planning mailing list
[hidden email]
https://lists.mozilla.org/listinfo/dev-planning