Making it easier to contribute non-functional changes

Ian Lynagh igloo at earth.li
Mon Oct 4 18:32:00 EDT 2010


On Mon, Oct 04, 2010 at 05:27:43PM -0400, Johan Tibell wrote:
> On Mon, Oct 4, 2010 at 4:49 PM, Ian Lynagh <igloo at earth.li> wrote:
> > FWIW, I graphed the number of patches per year in unix and base here:
> >    http://lambda.haskell.org/~igloo/lib_patches.png
> > (Library_submissions started in Oct 2006) but I'm not sure it really
> > tells us much, as it doesn't say how many patches were "maintenance"
> > rather than "development".
> 
> Thanks for putting together the graph. Do you have a feeling for how
> many patches are created by people without commit access?

No. But anyway, to get any meaningful information you'd need to compare
the proportions before and after Oct 2006. And there may be other
factors affecting the numbers too, of course.

> >> Non-functional changes should be allowed without review by the
> >> libraries list.
> >
> > As Ross said, simple, obvious improvements are applied without going
> > through the process. A common example is patches that just add haddock
> > docs to previously doc-less functions.
> 
> Does this apply to "obvious" improvements by the general Haskell
> community

Yes; I was thinking of haddock patches submitted by general Haskell
community members.

> > But you need some amount of review in order to know what a patch does.
> > For example, it was only when reading a recent patch
> >
> >    Sun Aug 29 13:02:45 BST 2010
> >      * Performance improvements to Data.Map
> 
> But that patch was sent to the libraries list for review, as it
> should. I was very much aware what changes the patch made!

I think you've missed my point.

Just because someone submits a patch that e.g. "Just adds some haddock
docs" doesn't mean they didn't accidentally include a small code change
or something too.

> >> Aside: Asking who has commit access in an email to the libraries list
> >> will quickly get you a link to the libraries process (I tried!).
> >
> > Well, if you want to get a patch committed, then that's normally what
> > you need, so that seems like a reasonable response to me.
> 
> But it will also make sure that people don't bother with smaller patches.

Well, trivial patches would be sent to libraries@ or put in the GHC
trac, so you still don't need to know who has commit access.

> >> I assume that contributor test their patches, just as they would test
> >> them before submitting them to a package not maintained by the
> >> libraries list.
> >
> > FWIW, it's quite common for submitted patches to break the build (e.g.
> > platform-specific breakage, or new code has warnings, or even just silly
> > mistakes that couldn't possibly compile (e.g. there was one in the past
> > week which added an unmatched "endif" to a makefile)).
> 
> libraries@ review doesn't change that, as people there don't normally
> check for these kind of problems (i.e. they don't actually compile and
> run the code).

I spotted the unmatched "endif" without compiling it. I also recently
found a segfault in the text package by looking at the source, rather
than by compiling/running it with random values. It's probably true that
most people don't review as carefully as I do, though.

But that point wasn't really about whether review would find problems;
rather, it was about the general quality of patches. (I don't mean to
put contributors off by pointing this out; just explaining why review is
important).

> We use a process for submitting code that's applicable to well
> polished libraries, but the libraries the process governs are anything
> but. For example, there are several of use who would like to make
> containers into a nice polished library, but I doubt we will actually
> do so if each patch needs to go through the libraries process.

But on the flipside, there was some anger recently when some containers
patches were pushed without review.


Thanks
Ian



More information about the Libraries mailing list