I've asked for many patches to be split up while I've been a core committer. Recently this has hit a couple nerves, so I'll explain a bit. Larger patches take more effort to review. There is more to test and more to think about. There are more places for bugs to hide and unintended changes to be hidden in. My job is to get as many good patches into the next release of Drupal as possible. My job is easier if patches are easier to review for myself and other reviewers. The different sections of a patch are usually of variable quality, some may go in immediately with only spot testing and some need thinking. I don't like rejecting good parts of patches because other parts need more work. When you are making a complex patch, weigh the effort for yourself to make smaller patches against the combined effort of reviewers (you should have at least two, someone else and then a core committer). Two examples: * PHP notice cleaning, which I like to see happen. Every change needs to be examined for correctness and potential for side effects. These patches usually have: - 3 to 10 individual changes - a pithy description like "Remove some PHP notices" - about half the changes are good and I'm not so sure about the others - no one else has reviewed it (I've already handled the ready-to-commits and criticals) If it were separated into smaller patches I could get half of the thing done and we could have specific discussion on the remainder. * Giant API or functionality changes, which I also like to see happen. These need to be somthing most people want and implemented well. Usually they: - are big and hard to be sure if everything relevant is tested. - have at least 50 follow-ups already, with less than one review per patch posted. - change one or two things as a side-effect of the main thing we should be reviewing. - always have bugs, some will be found after it is committed. - have a some interrelated changes that are big, complex, and inseparable. If the side-effect changes happened separately those could be committed faster and make the rest smaller and easier to review and test. For those still reading this rant that is too long, I think a plumbing analogy is appropriate. Think of the patch queue as a funnel with Dries and myself at the small end and over 200 patches in it at any given time. The larger patches clog the funnel and prevent them from being applied and sometimes block the path of other smaller patches. If patches are smaller they can flow through the funnel at a more steady and consistent rate, and get more things accomplished. If I am mistaken about the separation of a patch, then do what you do any other time I'm wrong (it happens)- provide a well-reasoned and concise response. There are no hard rules here since it is a bit subjective. -- Neil Drumm http://delocalizedham.com/
Somehow this turned into quite a long mail. ;) If you're not interested in the blah blah, please proceed to the "---" thing at the bottom. I think all the points you make are sound. Large patches are indeed more difficult to review, reviews take longer because of their potential for breaking stuff, and since so much is going on, there tends to be a lot of back-and-forth on the issue queue, which make reviews even more difficult. Arguments against it would be: 1. Every patch, even something seemingly simple and no-brainerish, is a possible victim for "analysis paralysis" which will hold up dependent patches and dependent functionality. For example, say merlinofchaos splits off the .meta files part of his new module administration patch page feature (http://drupal.org/node/ 76340), which one could conceivably say is a separate chunk of functionality easily submitted as a patch on its own. The whole issue could stagnate just because people can't agree on whether .meta files should instead become .install files or whether they should be written as .ini files or XML files or PHP files or... In the meantime, we miss this great piece of functionality that will bring Drupal administration to the next level because of these kinds of details which can always be changed later. 2. What to do about dependencies? In my fix signatures patch (http://drupal.org/node/10938), for example, the crux of the bug is just that forums should get signatures like their replies do. This is a one-liner fix. It cascades from there though, that comment's implementation of signatures is fundamentally buggy and if we copy that same behaviour to node module, now we've made two messes. Theoretically, each of the essentially 4 bugs that patch fixes could probably be moved into separate patches/issues. However, they're all inter-dependent. If nodes get comments too, then comment logic should be moved to user module, which is where the signature field come from. If we move signature appending to the form creation process to get around the current horrid "signature sitting in the add form" behaviour, it only follows that it should be in a themeable function. So then you get into rolling 4 different patches and either putting stuff like, "In order to test this patch, make sure you have applied #00000," or writing "brain-dead" patches that aren't at all what you have in mind for the final thing, just so they'll apply, and having to re-roll at least one of them when/if the other gets committed, etc. This also seems like it adds extra headaches all around, for the patch creator, the patch reviewers, AND the patch committer. 3. The code freeze draws very near. Patches that started back in July or so were much easier to break up into smaller chunks. You could have that 3-4 days of "review, needs work, review, needs work, review, RTBC" for every inter-dependent part of your patch and not have to worry that the "master plan" wouldn't get in. However, there have been some major API changes in just recently which have opened entirely new realms of possibilities for some patches, and going through that process on 18 smaller patches is a very dangerous thing for the prospective feature-adder, yet a lot of the functionality we've added will be only 1/2 or 3/4 implemented without these. I'm thinking particularly of patches like drupal_render() for profile display: http://drupal.org/node/79227 and Install-time and run-time requirements checking: http://drupal.org/ node/75002 --- I think a sensible rule of thumb would be "split up patches where you can," for the reasons you specify. But there are some places where it either just doesn't make sense, or worse makes things more difficult, and it would be great if core committers could keep that in mind when making these requests. I think another thing that would help *a lot* is to be given some more kind of concrete guidelines about what exactly CAN get in after the code freeze. It seems logical that relatively "minor" stuff like improved wording changes, some spiffy graphics to spruce things up, and so forth would be ok. "Usability" is a broad term, though: * Would something like http://drupal.org/node/80574 which would provide $form_values['preview'] and $form_values['submit'] be considered "usability"? (it certainly makes life easier for developers.) * Would Dries's patch for adding menu items to view 404s/403s: http:// drupal.org/node/79622 be considered "usability"? (It certainly makes life easier for site admins.) * Would a new default core theme be considered "usability?" (It certainly makes life easier for users, and for site designers who need a good Drupal CSS theme to start from.) etc. Then I think we would be able to draw better conclusions on where to focus our reviewing efforts, and we'd feel more at ease splitting up major patches, because we'd all be kind of on the same page as to what needed to get done by the 1st. If you got through all that, congrats and thanks for reading. ;) -Angie
Angela Byron wrote:
I think a sensible rule of thumb would be "split up patches where you can," for the reasons you specify. But there are some places where it either just doesn't make sense, or worse makes things more difficult, and it would be great if core committers could keep that in mind when making these requests.
Like all things, you have to be pragmatic. Feel free to push back if you think any request is unreasonable. (I do sometimes play devil's advocate to see how important some things are. Ber loves when I poke at removing various fields.)
I think another thing that would help *a lot* is to be given some more kind of concrete guidelines about what exactly CAN get in after the code freeze. It seems logical that relatively "minor" stuff like improved wording changes, some spiffy graphics to spruce things up, and so forth would be ok. "Usability" is a broad term, though:
I simply won't look at something labeled as a feature. If I think someone is mislabeling, I'll change it. Usability changes are often not simple rearrangements of items on a page (thats okay while frozen). Usability changes are perfectly justified in going all the way to the underlying database structure and APIs if thats what is needed (probably not okay while frozen). I'm not aware of any existing rules that are written down, but it does roughly correspond to issues that are bugs and simple tasks are okay to commit.
* Would a new default core theme be considered "usability?" (It certainly makes life easier for users, and for site designers who need a good Drupal CSS theme to start from.) etc.
I do not want to see some "base" theme get into core. We have default themeable functions that should see the markup improvements instead. Is anyone actually working on this stuff, admin or default theme, with the intention of getting it in core?
Then I think we would be able to draw better conclusions on where to focus our reviewing efforts, and we'd feel more at ease splitting up major patches, because we'd all be kind of on the same page as to what needed to get done by the 1st.
How I deal with this is: - Realize that everything will never be done. - And the list of things to do will always be about the same length. - There is always another release. So the point is to make things improve, not finish them.
If you got through all that, congrats and thanks for reading. ;)
Good rant. -- Neil Drumm http://delocalizedham.com/
Thanks for the reply, Neil! I'll try to reply to the rest of it in more detail tonight, but...
Is anyone actually working on this stuff, admin or default theme, with the intention of getting it in core?
Yes, Jeff Robbins and some other folks are working on a new default theme for core, with hopefully a demo/patch to come this weekend. To some extent, the holdup has been due to dependencies on some of the new theme stuff that have gone in recently, from what I understand.
On 25 Aug 2006, at 04:47, Angela Byron wrote:
For example, say merlinofchaos splits off the .meta files part of his new module administration patch page feature (http://drupal.org/ node/76340), which one could conceivably say is a separate chunk of functionality easily submitted as a patch on its own. The whole issue could stagnate just because people can't agree on whether .meta files should instead become .install files or whether they should be written as .ini files or XML files or PHP files or... In the meantime, we miss this great piece of functionality that will bring Drupal administration to the next level because of these kinds of details which can always be changed later.
This contrasts with my view. The reason the module administration patch isn't committed yet and why it is taking weeks (if not months) now, is because it does too much. It introduces: 1. _uninstall hooks, 2. a dependency system, 3. a notion of 'packages', 4. a new UI for module management and 5. meta files. Parts of this patch could probably be committed already, other parts need more discussion. In its current state, it is not likely to advance because it is such a burden for everybody involved. Unless this patch gets split, it is not likely to make the September 1 deadline. If this patch gets split, it won't be an 'all or nothing' scenario, and we are much likely to benefit from it in 4.8/5.0. -- Dries Buytaert :: http://www.buytaert.net/
Dries Buytaert wrote:
On 25 Aug 2006, at 04:47, Angela Byron wrote:
For example, say merlinofchaos splits off the .meta files part of his new module administration patch page feature
...
because it does too much. It introduces:
1. _uninstall hooks, 2. a dependency system, 3. a notion of 'packages', 4. a new UI for module management and 5. meta files.
Parts of this patch could probably be committed already, other parts need more discussion. In its current state, it is not likely to advance because it is such a burden for everybody involved. Unless this patch gets split, it is not likely to make the September 1 deadline. If this patch gets split, it won't be an 'all or nothing' scenario, and we are much likely to benefit from it in 4.8/5.0.
I just keep quietly asking for the throttle checkboxes for modules (and blocks) to be moved next to the throttle module configuration. That would: - Make the current page a lot simpler so future changes have something easier to build off of (dynamic numbers of columns are annoying). - Provide more space to actually explain what throttle does. (Does checked mean it is on or off while throttled?) - Better separation of code into modules. -- Neil Drumm http://delocalizedham.com/
I think a sensible rule of thumb would be "split up patches where you can," for the reasons you specify. But there are some places where it either just doesn't make sense, or worse makes things more difficult, and it would be great if core committers could keep that in mind when making these requests.
I see a tension between two principles: (1) we break up patches, and (2) each patch must be mature and complete before it's applied. These come into conflict when we have a large problem that needs a large solution. Yes, we can break the problem up, but each piece (patch) necessarily will be incomplete and immature until they're all done. Maybe to successfully break up large improvements we need an approach something like: (a) Signoff in principle in advance from a core committer: no guarantees, but the proposal seems the right general direction. And commitment from the group leading the change: we will follow through on cleanup after the patches go in. (b) Direction and agreement to break particular pieces off. (c) A relaxed readiness criterion for the pieces (individual patches) as they're applied. These are steps toward a goal. (d) Completion patches at the end to integrate the pieces. And I agree with both Neil and Angie. Some things are cleaner split off. And some things are so integral that they need at least a core that's large, though beyond that, sure, details can be hived off to be done separately/later.
On 25 Aug 2006, at 17:05, Nedjo Rogers wrote:
And I agree with both Neil and Angie. Some things are cleaner split off. And some things are so integral that they need at least a core that's large, though beyond that, sure, details can be hived off to be done separately/later.
I don't think we need complex rules here. Nor can people sign off things in advance. If it is impossible to split a patch, you can keep it as one patch. In all other cases, try to use common sense and go with the advise from the reviewer in case there are different opinions about it. I support Neil's case. Recently, there has been a trend to bundle bug fixes with feature requests (to help the features get in). -- Dries Buytaert :: http://www.buytaert.net/
I don't think we need complex rules here. Nor can people sign off things in advance.
I wasn't being clear. I don't mean rules, or formal sign-off. What I'm trying to capture is what sometimes currently happens in the patch queue, and is successful. There are (informal) agreements and commitments about process that allow us to break up a larger patch. A core committer says: the approach is looking good, it has a good chance of making it in if changes can be made. The individual or (better) group leading the change says: okay, we'll work on these changes, maybe in separate issues as suggested, and we'll do the followup that's needed. I see a couple of common weak points in this process, though. 1. Broken up pieces (separate patches) probably need a slightly different set of criteria, as they are steps toward a goal. We're not always clear about this. 2. Broken up pieces need a commitment to followup. As individuals or groups leading a change, we're often weak on followup. The immediate change gets in and we move on to something new and more exciting than cleanup. So, to work well, the "break it up into pieces" approach relies on informal agreements and commitments and trust from both the contributors and the core committers. The core committers need to go somewhat on faith, and the contributors need to follow through. Ideally, this is mapped out a bit in advance, there in the patch queue, so we know the game plan and what we're all committing to.
On Fri, 25 Aug 2006, Nedjo Rogers wrote:
Maybe to successfully break up large improvements we need an approach something like: (a) Signoff in principle in advance from a core committer: no guarantees, but the proposal seems the right general direction. And commitment from the group leading the change: we will follow through on cleanup after the patches go in. (b) Direction and agreement to break particular pieces off. (c) A relaxed readiness criterion for the pieces (individual patches) as they're applied. These are steps toward a goal. (d) Completion patches at the end to integrate the pieces.
The point is that most separated patches are nice improvements on their own, even if other separated patches from the original big one will not make it into core. A possible practice is what our i18n to core effort is trying to achieve. (http://drupal.org/node/77866) First one big patch to debate the concept and then possibly smaller patches to help reviewers and comitters. Goba
Op vrijdag 25 augustus 2006 01:05, schreef Neil Drumm:
If I am mistaken about the separation of a patch, then do what you do any other time I'm wrong (it happens)- provide a well-reasoned and concise response. There are no hard rules here since it is a bit subjective.
No, you are completely right. In the current way of Drupalleering it is near to impossible to get any big changes in in a clean way. Even for the maintainers of patches: small patches require far less work if some other patch got committed in the mean time. The problem, however lies deeper, IMO. Because our system/workflow "prefers" small patchesm, we, the developers tend to put on horselflaps when developing. If I ever encounter an api that has an inconsistent nqme, there is really no F-ing way that I will change that API, when all I want is introduce a new feature that simply calls that api. Even worse: our system/worflow makes it easier to rather write yet another API for the feature you want to introduce, then to overhaul the three existing APIS and merge them into one, far more usefull api. What I am trying to point out, is not that we should go for bigger, more inclusive patches. I think Drumm explained very well that that is not really a good option. I am trying to put my finger on a (very) sore point in Drupal. Drupal that rewards small/tiny hacks, but punishes those who prefer general and more constructive solutions. The netto result is that we see small improvements happen all over the place. But that API improvements, archetectural improvements never happen, untill the day that it bursts, the day that we get FAPI alike improvements. I would like to find a solution inbetween. Some system that rewards someone/somthing that makes, for example, the taxonomy apis consistent with the node apis. Something that rewards a person who took the time to make the (plural/singular) naming of our tables consitent. I am searching for a way to "keep the house clean inbetween the yearly spring-cleanings". Only diswashing once a day is not enough, you need to do some cleaning on weekly base too. If you don't you will only make the spring-cleaning a hell. Bèr Bèr
participants (6)
-
Angela Byron -
Bèr Kessels -
Dries Buytaert -
Gabor Hojtsy -
Nedjo Rogers -
Neil Drumm