[development] RTBC - how does it work?
gabor at hojtsy.hu
Tue Jul 3 20:12:42 UTC 2007
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,
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.
More information about the development