On Mon, Oct 4, 2010 at 4:49 PM, Ian Lynagh
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? Quite a few patches to these libraries are made by e.g. Simon M.
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 or only by a set of core contributes (e.g. GHC developers). If one believes one has such an improvement, is there a description of how to get it committed?
Note that some patches are obvious when being created, but are hard to sanity check afterwards. For example, when making a patch that just moves functions around, it's easy to be confident it's safe when creating the patch, but when someone else sees it it just looks like a load of code being added and removed. It's even worse if other changes are made in the same patch.
From my experience these things are not being looked at by people on libraries@ so I don't think having a public review of non-functional patches decreases the likelihood of breakages. A contributor could of course ask for a review if he/she thinks it's necessary. The commiter could also have a look to make sure things don't break.
In general, it makes life easier for the committers if the libraries process is used, so it is not down to us to review the patches.
I understand that, but it also seems to prevent us from cleaning up our base libraries.
or attached to a "please merge" ticket on the bug tracker.
That's not what 'merge' is for. I improved the description a couple of weeks ago, so it's hopefully clearer now: fixed in HEAD; please merge to STABLE Next status will be 'merge'
The right state is 'patch': please review Next status will be 'patch'
Got it. Thanks for the clarification.
Only changes that don't affect the API or performance (in non-trivial ways) should be allowed in without a review.
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!
It's possible that once a commit that should have been reviewed gets commited. I suggest we adopt an "ask for forgiveness, not for permission" policy in these cases. We can always roll back changes.
IIRC, part of the motivation for the library process was that changes were being made in the HEAD, getting into release, and then people discovered that they caused problems.
Perhaps we could have a commit mailing list like Sterling suggested?
someone (e.g. Ian)
You mentioned my name a few times; I'm not too keen on proposals that mean more work for me!
I didn't intend it to! Read that as <whoever actually has commit access and commits stuff>.
If you do have a patch that doesn't need to go through the libraries process, the best thing to do is to file a ticket and set its status to 'patch'. Assigning it to me is unlikely to get it applied faster, but means no-one else is likely to look at it.
Sure. That sounds reasonable. As part of this proposal it would be nice if we could clarify who does have commit access to our core libraries. I have no idea and I doubt most people have.
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.
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). 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. Thanks, Johan