Review of patches, etc.

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

Review of patches, etc.

Christopher Schmidt-2
I'm beginning to think I was a bit overzealous in stating that all
patches should require an external review. I've been watching OpenLayers
development lately, and although I agree that in principle, code review
is a good thing, I think that it has seriously hamstrung the project
over the past couple months, due to lack of development time from a few
core committers who were able to help move things forward in the past.

Getting more core committers is one helpful step in the right direction,
but it doesn't completely solve the problem: we're still blocking on
very simple (one line) patches due to lack of review.

I'd like to float the idea of allowing commits to trunk without review
at the discrestion of the patch author. We've got really great
committers, and I think we have the ability to decide when things
should/shouldn't go in without a review on every patch.

That said, when API functionality is involved, I think more thna one
person should be able to comment before it goes into trunk. So, I think
that any patch which adds new APIMethods or APIProperties should
probably be opened as a ticket, and comments should be sought on it
before it goes into trunk. Additionally, we should be more proactive in
discussing things both in tickets nd on the mailing lists when there is
a suggestion in API change.

How do other people feel about this? I don't want to continue to see one
line patches left open for 6 months due to lack of review.

Regards,
--
Christopher Schmidt
MetaCarta
_______________________________________________
Dev mailing list
[hidden email]
http://openlayers.org/mailman/listinfo/dev
Reply | Threaded
Open this post in threaded view
|

Re: Review of patches, etc.

John Cole X
Christopher,
  I tend to agree with you.  I've put in several patches, most I consider
very non-invasive.  Some have been looked at, some haven't.

  OL is still a very young product, and I think it's ok to have the trunk be
a little wild. :-)  In fact, one thing that prevents me from using the trunk
for normal development is the fact that I have so many changes to 2.5 that
aren't in the trunk (that I think could be), that I need a stable version to
base my changes off of, rather than getting caught by a fix/change that
hoses me.  

  It's entirely possible that there are several really good teams using OL
but are off on their own branch or maintaining a locally changed copy (like
me), preventing the groups best eyes from being on the trunk.

John

-----Original Message-----
From: [hidden email] [mailto:[hidden email]] On
Behalf Of Christopher Schmidt
Sent: Friday, December 14, 2007 6:52 AM
To: [hidden email]
Subject: [OpenLayers-Dev] Review of patches, etc.

I'm beginning to think I was a bit overzealous in stating that all
patches should require an external review. I've been watching OpenLayers
development lately, and although I agree that in principle, code review
is a good thing, I think that it has seriously hamstrung the project
over the past couple months, due to lack of development time from a few
core committers who were able to help move things forward in the past.

Getting more core committers is one helpful step in the right direction,
but it doesn't completely solve the problem: we're still blocking on
very simple (one line) patches due to lack of review.

I'd like to float the idea of allowing commits to trunk without review
at the discrestion of the patch author. We've got really great
committers, and I think we have the ability to decide when things
should/shouldn't go in without a review on every patch.

That said, when API functionality is involved, I think more thna one
person should be able to comment before it goes into trunk. So, I think
that any patch which adds new APIMethods or APIProperties should
probably be opened as a ticket, and comments should be sought on it
before it goes into trunk. Additionally, we should be more proactive in
discussing things both in tickets nd on the mailing lists when there is
a suggestion in API change.

How do other people feel about this? I don't want to continue to see one
line patches left open for 6 months due to lack of review.

Regards,
--
Christopher Schmidt
MetaCarta
_______________________________________________
Dev mailing list
[hidden email]
http://openlayers.org/mailman/listinfo/dev

No virus found in this incoming message.
Checked by AVG Free Edition.
Version: 7.5.503 / Virus Database: 269.17.1/1183 - Release Date: 12/13/2007
9:15 AM
 

No virus found in this outgoing message.
Checked by AVG Free Edition.
Version: 7.5.503 / Virus Database: 269.17.2/1184 - Release Date: 12/14/2007
11:29 AM
 
This email and any files transmitted with it are confidential and intended solely for the use of the individual or entity to whom they are addressed. If you have received this email in error please notify the sender. This message contains confidential information and is intended only for the individual named. If you are not the named addressee you should not disseminate, distribute or copy this e-mail.
_______________________________________________
Dev mailing list
[hidden email]
http://openlayers.org/mailman/listinfo/dev
Reply | Threaded
Open this post in threaded view
|

Re: Review of patches, etc.

Jachym Cepicky
In reply to this post by Christopher Schmidt-2
Hi,
it speeds up the development - do not fear and go ahead. People do
usually  distinguish between small patch and big change - they will
break something only sometimes :-)

Jachym

Christopher Schmidt píše v Pá 14. 12. 2007 v 07:52 -0500:

> I'm beginning to think I was a bit overzealous in stating that all
> patches should require an external review. I've been watching OpenLayers
> development lately, and although I agree that in principle, code review
> is a good thing, I think that it has seriously hamstrung the project
> over the past couple months, due to lack of development time from a few
> core committers who were able to help move things forward in the past.
>
> Getting more core committers is one helpful step in the right direction,
> but it doesn't completely solve the problem: we're still blocking on
> very simple (one line) patches due to lack of review.
>
> I'd like to float the idea of allowing commits to trunk without review
> at the discrestion of the patch author. We've got really great
> committers, and I think we have the ability to decide when things
> should/shouldn't go in without a review on every patch.
>
> That said, when API functionality is involved, I think more thna one
> person should be able to comment before it goes into trunk. So, I think
> that any patch which adds new APIMethods or APIProperties should
> probably be opened as a ticket, and comments should be sought on it
> before it goes into trunk. Additionally, we should be more proactive in
> discussing things both in tickets nd on the mailing lists when there is
> a suggestion in API change.
>
> How do other people feel about this? I don't want to continue to see one
> line patches left open for 6 months due to lack of review.
>
> Regards,
--
Jachym Cepicky
e-mail: [hidden email]
URL: http://les-ejk.cz
GPG: http://www.les-ejk.cz/pgp/jachym_cepicky-gpg.pub


_______________________________________________
Dev mailing list
[hidden email]
http://openlayers.org/mailman/listinfo/dev

signature.asc (196 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Review of patches, etc.

Paul Spencer-2
In reply to this post by Christopher Schmidt-2
Chris,

I think this is a good idea.  I think the benefits of attracting more  
contribution are worth giving a little on process.

Paul

On 14-Dec-07, at 7:52 AM, Christopher Schmidt wrote:

> I'm beginning to think I was a bit overzealous in stating that all
> patches should require an external review. I've been watching  
> OpenLayers
> development lately, and although I agree that in principle, code  
> review
> is a good thing, I think that it has seriously hamstrung the project
> over the past couple months, due to lack of development time from a  
> few
> core committers who were able to help move things forward in the past.
>
> Getting more core committers is one helpful step in the right  
> direction,
> but it doesn't completely solve the problem: we're still blocking on
> very simple (one line) patches due to lack of review.
>
> I'd like to float the idea of allowing commits to trunk without review
> at the discrestion of the patch author. We've got really great
> committers, and I think we have the ability to decide when things
> should/shouldn't go in without a review on every patch.
>
> That said, when API functionality is involved, I think more thna one
> person should be able to comment before it goes into trunk. So, I  
> think
> that any patch which adds new APIMethods or APIProperties should
> probably be opened as a ticket, and comments should be sought on it
> before it goes into trunk. Additionally, we should be more proactive  
> in
> discussing things both in tickets nd on the mailing lists when there  
> is
> a suggestion in API change.
>
> How do other people feel about this? I don't want to continue to see  
> one
> line patches left open for 6 months due to lack of review.
>
> Regards,
> --
> Christopher Schmidt
> MetaCarta
> _______________________________________________
> Dev mailing list
> [hidden email]
> http://openlayers.org/mailman/listinfo/dev

+-----------------------------------------------------------------+
|Paul Spencer                          [hidden email]    |
+-----------------------------------------------------------------+
|Chief Technology Officer                                         |
|DM Solutions Group Inc                http://www.dmsolutions.ca/ |
+-----------------------------------------------------------------+





_______________________________________________
Dev mailing list
[hidden email]
http://openlayers.org/mailman/listinfo/dev
Reply | Threaded
Open this post in threaded view
|

Re: Review of patches, etc.

John R. Frank

Chris, sounds like a good idea.

Is it sufficient to request that the committer make detailed comments in
the SVN commit -m?  Or should every one-liner have a ticket opened and
closed by the committer?

John


On Sat, 15 Dec 2007, Paul Spencer wrote:

> Chris,
>
> I think this is a good idea.  I think the benefits of attracting more
> contribution are worth giving a little on process.
>
> Paul
>
> On 14-Dec-07, at 7:52 AM, Christopher Schmidt wrote:
>
> > I'm beginning to think I was a bit overzealous in stating that all
> > patches should require an external review. I've been watching
> > OpenLayers development lately, and although I agree that in principle,
> > code review is a good thing, I think that it has seriously hamstrung
> > the project over the past couple months, due to lack of development
> > time from a few core committers who were able to help move things
> > forward in the past.
> >
> > Getting more core committers is one helpful step in the right
> > direction, but it doesn't completely solve the problem: we're still
> > blocking on very simple (one line) patches due to lack of review.
> >
> > I'd like to float the idea of allowing commits to trunk without review
> > at the discrestion of the patch author. We've got really great
> > committers, and I think we have the ability to decide when things
> > should/shouldn't go in without a review on every patch.
> >
> > That said, when API functionality is involved, I think more thna one
> > person should be able to comment before it goes into trunk. So, I
> > think that any patch which adds new APIMethods or APIProperties should
> > probably be opened as a ticket, and comments should be sought on it
> > before it goes into trunk. Additionally, we should be more proactive
> > in discussing things both in tickets nd on the mailing lists when
> > there is a suggestion in API change.
> >
> > How do other people feel about this? I don't want to continue to see
> > one line patches left open for 6 months due to lack of review.
> >
> > Regards,
> > --
> > Christopher Schmidt
> > MetaCarta
> > _______________________________________________
> > Dev mailing list
> > [hidden email]
> > http://openlayers.org/mailman/listinfo/dev
>
> +-----------------------------------------------------------------+
> |Paul Spencer                          [hidden email]    |
> +-----------------------------------------------------------------+
> |Chief Technology Officer                                         |
> |DM Solutions Group Inc                http://www.dmsolutions.ca/ |
> +-----------------------------------------------------------------+
>
>
>
>
>
> _______________________________________________
> Dev mailing list
> [hidden email]
> http://openlayers.org/mailman/listinfo/dev
>
_______________________________________________
Dev mailing list
[hidden email]
http://openlayers.org/mailman/listinfo/dev
Reply | Threaded
Open this post in threaded view
|

Re: Review of patches, etc.

Christopher Schmidt-2
On Sat, Dec 15, 2007 at 08:56:32AM -0500, John R. Frank wrote:
>
> Chris, sounds like a good idea.
>
> Is it sufficient to request that the committer make detailed comments in
> the SVN commit -m?  Or should every one-liner have a ticket opened and
> closed by the committer?

I don't think we need tickets for everything. Any significant changes --
changes that you would want mentioned in a changelog -- should end up
with a ticket, because that's how we track for mentioning them in
release notes/news for the next release. Sufficiently detailed commit
messages seem fine to me for the common case where a bug is fixed 'on
the fly' -- it will likely be a single line change that this kind of
thing is affected for.

So yeah, I'd say it only needs a ticket if you think that it's something
that's going to be mentioned in release notes. Anything that's simple
bugfixing doesn't need that.

Of course, tickets should continue to be opened by users who know a
problem, but not a fix :) Or with patches for those who don't have
commit rights.

Regards,
--
Christopher Schmidt
MetaCarta
_______________________________________________
Dev mailing list
[hidden email]
http://openlayers.org/mailman/listinfo/dev
Reply | Threaded
Open this post in threaded view
|

Re: Review of patches, etc.

Cameron Shorter
+1. My experience with Mapbuilder is that developers are generally
respectful and good at identifying when to ask for comment and when to
"just fix the typo".

Christopher Schmidt wrote:

> On Sat, Dec 15, 2007 at 08:56:32AM -0500, John R. Frank wrote:
>  
>> Chris, sounds like a good idea.
>>
>> Is it sufficient to request that the committer make detailed comments in
>> the SVN commit -m?  Or should every one-liner have a ticket opened and
>> closed by the committer?
>>    
>
> I don't think we need tickets for everything. Any significant changes --
> changes that you would want mentioned in a changelog -- should end up
> with a ticket, because that's how we track for mentioning them in
> release notes/news for the next release. Sufficiently detailed commit
> messages seem fine to me for the common case where a bug is fixed 'on
> the fly' -- it will likely be a single line change that this kind of
> thing is affected for.
>
> So yeah, I'd say it only needs a ticket if you think that it's something
> that's going to be mentioned in release notes. Anything that's simple
> bugfixing doesn't need that.
>
> Of course, tickets should continue to be opened by users who know a
> problem, but not a fix :) Or with patches for those who don't have
> commit rights.
>
> Regards,
>  


--
Cameron Shorter
Geospatial Systems Architect
Tel: +61 (0)2 8570 5050
Mob: +61 (0)419 142 254

Think Globally, Fix Locally
Commercial Support for Geospatial Open Source Solutions
http://www.lisasoft.com/LISAsoft/SupportedProducts.html

_______________________________________________
Dev mailing list
[hidden email]
http://openlayers.org/mailman/listinfo/dev
Reply | Threaded
Open this post in threaded view
|

Re: Review of patches, etc.

Tim Schaub-3
In reply to this post by Christopher Schmidt-2
Hey-

I doubt this is going to be popular, but in general, I'm against the
idea of removing the ticket/review process.

Sure, if someone spots a typo, we want to make it easy to fix that.  But
I don't think typos are really what we're talking about here.

I like two things about OpenLayers:
1) The trunk works.
2) We encourage paired programming or at least some discussion with
other developers to make sure modifications are well thought out.

I think the first is really a consequence of the second.  I rely heavily
on a working trunk.  I don't look forward to working on a project where
this is a liability.

More importantly, I really (really) like the collaborative effort on
modifications to the trunk.  Sandboxes are a fun and easy way for a
single developer (or a group of developers) to take off in their own
direction with an idea.  I like that bringing those modification in to
the trunk takes time.  More significantly, I like that it also takes the
eyes of other core developers.

I know there is a wide range between fixing typos and merging major
sandbox features.  However, with the exception of very trivial typo
fixes, I think all modifications benefit from more than one set of eyes.

I'm entirely in favor of making it easier to review and commit changes.
  However, I am against changing our ticket/review policy because I'm
not interested in working on a project that is really just a bunch of
independent minded developers slapping together things that look right
to them.

I look forward to working out a policy that makes everyone happy - and
I'm confident that we can.  (I also look forward to having Erik weigh in
on this before we make any changes.)

Tim

Christopher Schmidt wrote:

> I'm beginning to think I was a bit overzealous in stating that all
> patches should require an external review. I've been watching OpenLayers
> development lately, and although I agree that in principle, code review
> is a good thing, I think that it has seriously hamstrung the project
> over the past couple months, due to lack of development time from a few
> core committers who were able to help move things forward in the past.
>
> Getting more core committers is one helpful step in the right direction,
> but it doesn't completely solve the problem: we're still blocking on
> very simple (one line) patches due to lack of review.
>
> I'd like to float the idea of allowing commits to trunk without review
> at the discrestion of the patch author. We've got really great
> committers, and I think we have the ability to decide when things
> should/shouldn't go in without a review on every patch.
>
> That said, when API functionality is involved, I think more thna one
> person should be able to comment before it goes into trunk. So, I think
> that any patch which adds new APIMethods or APIProperties should
> probably be opened as a ticket, and comments should be sought on it
> before it goes into trunk. Additionally, we should be more proactive in
> discussing things both in tickets nd on the mailing lists when there is
> a suggestion in API change.
>
> How do other people feel about this? I don't want to continue to see one
> line patches left open for 6 months due to lack of review.
>
> Regards,

_______________________________________________
Dev mailing list
[hidden email]
http://openlayers.org/mailman/listinfo/dev