John Wilkins wrote:
On Jul 3, 2007, at 11:00 AM, Dries Buytaert wrote:
Despite some frustration during the code freeze, this looks very much like a job well done. Therefore, I don't think the system is fundamentally broken or doomed as some people try to make us believe. There is room for improvement, but let's not neglect the fact that we did a really great job.
The people who are frustrated don't think the process is doomed; I haven't seen anyone say or imply this. But many DO think that the current implementation of the RTBC status is fundamentally flawed, since it doesn't mean what it says.
As many people noted, it means: "ready for committer review". It does not mean that a committer robot will come around and commit the code automatically. Some people think it means the robot.
And I'm happy with the idea of a hard code freeze deadline. But not happy with issues that appear to meet the deadline, but then get over-looked, not committed and not commented on.
Meeting the deadline equals "being committed". Patches not committed did not meet the deadline. Most patches I handled required at least half an hour to review, but often it was more. Many times neither the original issue description, neither a summary in the comments helped to understand - why the patch is important for Drupal - what was not adequate before, which needed modification - what the patch does / changes Not knowing all these, it took a lot longer to review patches. There were occasions when I reviewed a patch for an hour, then my only option was to post a follow up to ask the above questions. (Unfortunately I was sometimes guilty of the same issues, I got similar questions on some of my D6 patches from Dries). Additionally to the above "marketing" issues, some common yet basic source code level problems even with RTBC patches were: - too many "unrelated" stuff in one patch, AKA feature creep - does not apply because of other changes made in the meantime - missing or nearly missing or hard to understand phpdoc comments - coding style errors (variable naming, function naming, concatenation, tabs) I don't think these need a core committer to identify. I think I can speak for Dries too, that many patches stop at the "why we need this point" and need more thought before can actually be looked into. Patch creators are not the best marketing people of course, but that would help a lot here. Also, as Jeff Eaton explained, it is hard to be "the bad guy". A patch author rarely questions why you set her patch to RTBC, but she will surely question if you set it to "code needs work", you need to provide good reasons. Deep reviews take time (and on many occasions result in CNW), bumping a patch up often happens more easily without actual testing or deep thinking behind it. It often takes a lot of time for someone to set patches to a non-RTBC status, because it needs a lot of thought. You need to understand the problem, you need to understand the solution, look at the use cases, think about contrib reuse possibilites/challenges, you need to compare it to other things done in Drupal (do not reinvent the wheel), you need to check the code, look for performance, security and other kinds of problems, and then share your negative findings with the other developers. Of course if there are no negative findings, a core comitter can just as well commit the code, so the reviewer can explain what she looked at, what she liked, why she thinks that it is good to go. By bumping patches into RTBC easily, and expecting core comitters to take their time with all these instead of doing it before marking a patch RTBC, the RTBC state is devalued. It does not mean "ready for being comitted" because how it is used. How core committers look at it is a consequence, not a cause. Gabor