[OpenLayers-Dev] Ticket #1093

[Patrick Valsecchi]

Christopher,

Thank you very much for this thorough answer.

I'll take all those tips in concideration the next time I'll have a ticket to 
write for OL.

I'm happy to ear the time to commit has been improved for OL, I hope I'll have 
the occasion to test that in the future. ;-)

As of asking directly my co-workers, I prefer not to, expect for simple 
comment fixes that don't require tickets. The point of the code review is to 
make sure the change serves everybody's interest and not only one's company. 
But if you say it doesn't bother you, I'll do it more often.

Have a nice day.

On Thursday 29 January 2009 13.56:08 you wrote:
> On Thu, Jan 29, 2009 at 08:32:17AM +0100, Patrick Valsecchi wrote:
> > Hi,
> >
> > About this ticket:
> > http://trac.openlayers.org/ticket/1093
> >
> > I see there are a few missunderstanding. So I'll try to answer to your
> > last comment.
> >
> > 1) The quote patch unquote scandal
> >
> > I tend to put the two liners inline because that way, I don't have to
> > create a file, attach it to the ticket and you, you don't have to click
> > on the attachment, click on "raw", save the file, go to your shell and
> > apply the patch.
>
> I work on OpenLayers code in a particular way. The way I do it is:
>  * Open this page:
>    http://trac.openlayers.org/wiki/Release/2.8
>  * Go through the list of tickets marked 'review'.
>  * Select a ticket
>  * Scroll to the attachments section
>  * Right click an attachment, click 'copy link'
>  * Go to my terminal, type
>     curl '<ctrl-v-to-paste>?format=raw' | patch -p0
>  * Run the new tests, make sure they work
>  * svn revert -R lib
>  * Run the new tests, make sure they fail.
>
> Any patch that I can do this for, and feel confident solves the problem
> without causing other ones, is usually quickly committed. (This has
> changed in the 2.8 timeframe, as I am no longer making as much of an
> effort to review every patch.)
>
> Any patch that I can not do this for gets bumped below any ticket I can
> do this for.
>
> > Did you know that commands comming from the UNIX world tend to expect
> > the data to come from STDIN when no file is specified? That way, you
> > can just type "patch -p0", press enter, cut&paste the quote patch
> > unquote in your shell, press enter and finally CTRL+D.
>
> Sure. And you have pasted only one section of 'working' code into this
> ticket, which is an invalid solution, because it would fix MapServer and
> break WMS.
>
> > I think it's easier that way for both of us. Don't you?
>
> Using my review process, I would *never see* #1093. It was never marked
> for review.
>
>   http://trac.openlayers.org/wiki/CreatingPatches
>
> Is quite clear about the way to get a ticket seen by me. Anyone who
> doesn't follow that process is likely to have their patch missed by me.
>
> > 2) The lack of final patch from me
> >
> > In your "10/17/07 09:40:58" comment, you said you wanted to see with the
> > MapServer developers first. Since it's a pain in the ass for me to write
> > OL patches, I was waiting for the result of this.
>
> I'm sorry, I never clarified after that. Several days later, I confirmed
> the fact that MapServer's extent calculation was broken, and I didn't
> update the ticket. This was very much my fault, and I apologize -- it
> simply slipped my mind, and the patch never again made it up the review
> queue because -- again -- it was never marked for Review, and the only
> way I ever look at tickets is if they are marked for review. (If the
> review queue is *empty*, I keep looking, but it hasn't been empty since
> before 2.6.)
>
> > Why is it so hard for me to write a patch? In the past, all the OL patch
> > I submitted where either rejected or took ages to be committed. Since I
> > need the correction fast to be able to move forward in my job, I have to
> > first change the buggy methods using javascript overriding. That way i
> > can do the work I'm paid for. Then, I write a ticket, modify the OL code
> > to create a patch. After that, i have to go through the pain to argue why
> > I've used solution A instead of B, apply some unwroten coding convention
> > to my patch (a missing space here and there) and, maybe, after a few
> > patch versions and a couple of month of arguing, the modification appears
> > on SVN. Then, I have to remove my "JS override" patch from my code.
>
> You have, at this point, access to at least three committers on the
> OpenLayers project within your company. This ticket was only ever
> replied to by me. No one else even bothered to offer an opinion. In this
> particular case, there was a social mistake I made -- over a year ago --
> which may have negatively affected thigns, and I apologize again for
> that. I did hold up the process on my end, and it's perfectly reasonable
> to blame me for that, and that *may* be why no one else has commented.
>
> However, with those resources at hand, I'll notice that myself -- anad
> Erik, acting as release triage team -- are the only ones who have ever
> touched this ticket. To me, all this indicates is a lack of free time
> on the part of developers to review the problem.
>
> Clearly, review of problems is neccesary, because the patch you attached
> was *broken*. The change was the wong one to make, and no further change
> was obviously suggested.
>
> Because the issue is minor and affects few people, this is not a high
> developer priority -- as you said, using MapServer layers in general is
> not recomended, so it doesn't get attention.
>
> But when I'm the only person commenting, what this reads like to me is a
> direct commentary that there are not sufficient resources to look at
> every ticket in trac in a timely manner and deal with them.
>
> Since Camptocamp *has* resources available devoted to the OpenLayers
> project, but did not use them to look at this ticket, the only thing I
> can take from that is that the fix is not important to Camptocamp. I'll
> be straightforward here: If an app which is being developed needs a fix,
> and the company developing it has resources to commit a fix itself, I
> will generally not make it my *personal* priority to make it easier for
> the company to continue in their project. There is *no reason* that
> one of your coworkers could not have reviewed this patch.
>
> This applies for other aspectso f the project as well. For example, the
> MapGuide support is primarily developed by DM Solutions, primarily by
> Mike Adair. My personal motivation to work on this is low -- the primary
> user, DM Solutions, has access to at least two competent Javascript
> coders who can write patches to the code (Zac and Mike), and one who can
> review them and commit them (Paul Spencer). Since my knowledge of
> MapGuide is low, I leave MapGuide related tickets to be handled by the
> people who know them -- primarily DM Solutions. As a result, these
> patches tend to linger, because DM doesn't have the resources to devote
> to them. Since it doesn't affect users that I interact with, this
> doesn't move up my priority list.
>
> I'll note that your original responses to this ticket took place within
> *hours* -- much better turnaround time than I've gotten from any other
> client. In fact, I have two minor patches to Webkit which have been
> outstanding for two years, and regularly find that tickets I write in
> other Open Source projects are left untouched for long periods of time.
>
> OpenLayers isn't perfect. We are extremely developer-time limited, and
> do not have sufficient resources to attack everything we get -- not even
> everything with patches. There are many patches which I hae seen
> languish which are much more useful than a minor change to something
> that no one uses.
>
> My priorities are two fold:
>  1. Corporate. Typically high priority, but limited in scope. These are
>     the things I need to fix for MetaCarta. This happens approximately
>     once every 6 months.
>  2. Personal. I approach problems that interest me, and I think will
>     have significant benefit to the project. This is the lion's share --
>     probably about 95% of my time on OpenLayers over the past year.
>
> It is my belief that there is no other trunk contributor who, at this
> point in time, spends much of the latter time on OpenLayers. As a
> result, the concentration is on things that affect the work they are
> doing on the projects they're paid to work on.
>
> This is how things happen. There's nothing to do about that -- money
> talks. But if you really have a need within your company to fix
> something, then drop an email to someone within your company, and get
> them to support the work. Even an additional comment of "This is
> affecting me and I can confirm that solution $x works without breaking
> anything" would completely change the game as far as getting my
> *personal* attention goes, because I devote my personal attention to
> things that I see affecting people.
>
> Some recent examples:
>
>   http://trac.openlayers.org/ticket/1908 convenience method for
>   geometry.fromWKT.
>     I need this pretty much every time I experiement with vectors.
>     Patch uploaded with tests, reviewed and marked commimt within 5
>     hours, committed within 6.
>
>  http://trac.openlayers.org/ticket/1822  Vector getDataExtent modiies
>    feature
>    Wrote testss, committed within 2 days
>
>  http://trac.openlayers.org/ticket/1872 minor improvement to
>  getMousePosition
>    committed within 1 day
>
> Some of these are me, some of these are elemoine, but all of them have a
> significant difference: they followed the standard pipeline, and they
> were things we cared about.
>
> If you want something to get done, *make a developer care*. You can do
> this by paying them, you can do this by making them interested -- but
> you can't do this by uploading incorrect patches, not following up, not
> following process -- and then getting upset that they don't review your
> patches.
>
> I have seen many good contributors get patches approved in minutes or
> hours -- from Camptocamp, from the OpenStreetMap project, from OpenGeo,
> from independant contractors working on the project. I have seen many
> fine patches languish, even after following the procedure, and uploading
> a perfectly reasonable patch with tests and supporting evidence that the
> code is correct.
>
> However, when these are not the case -- when a patch doesn't start with
> sufficient supporting evidence, when it is in a hard to understand grey
> area, when it is not marked for review, when it doesnt have tests...
> all of these cause it to get lower in my personal todo list. I expect
> other developers are much the same.
>
> > Why is this process so long? I'll let you answer to that one. I've
> > submitted patches to a lot of other OSS projects and OL is the most
> > painful.
>
> In this case, the process was long because no one has ever created a
> patch to the code which fixes the problem and uploaded it to trac --
> even as a comment. Not to mention that I expect that this patch would
> break a number of tests -- which is not addressed in this ticket at all.
>
> In general, the process is short when your code is succinct and
> well-explained. It is also short when it has tests, since that lets us
> confirmr quickly and easily that the behavior is correct in the future.
> It is also short when a developer -- or even another user -- is
> interested in it, because then a second pair of eyes can be attached to
> the code, and give it a htumbs up or thumbs down.
>
> In this case, you have none of these. At least one of them is
> potentially more within your control than mine -- you have social
> relationships of some sort with at least some number of other developers
> and users of the code, who could comment and say that they'd like to see
> the fix.
>
> OpenLayers is all about scratching developer itches. My itches come from
> helping as many users as I can. Your patch didn't scratch that itch --
> or any other itch of mine. That's unfortunate. To get patches approved,
> you really have to scratch an itch -- or fight hard to prove that it
> will become an itch if not scratched.
>
> Best Regards,



-- 
Patrick Valsecchi
Senior Software Engineer
Camptocamp Suisse SA
http://www.camptocamp.com
+41 21 619 1030