Hi Chuck,
Here is my reply for your general comments regarding style of work etc...
cbell@stripped wrote:
> Rafal,
>
> I am disappointed that you regard the kernel with such high emotional
> protectiveness. I understand most of your points you outlined but I
> think to work effectively we must devoid ourselves from certain
> prejudices regarding some types of changes to the code.
>
I do treat backup kernel emotionally because I wrote this code - I think this is
understandable. But I don't think I have any prejudices. As I explain below I
think I had right to ask for the changes you introduced and that would be the
case regardless of what part of the code you were changing. The only thing is
that I know backup kernel better than other parts of the code and it is easier
for me to spot problems there.
> I am ok with you suggesting changes to how parameters are passed and
> such -- that's not what I am complaining about. What bothers me most is
> that it seems any time I make a change to any part of the kernel it's
> never good enough to pass your reviews and it takes a minimum of four
> reviews to satisfy you. We all must simply learn to relax a little! :)
>
> Since you feel so strongly about changing the parameter names, types,
> etc., please feel free to make those changes -- just have Svoj or
> someone review them first.
I don't feel strong against changing parameter names or types. I feel strong
against changing them without a reason - this is different. I might not see or
understand the reasons behind some changes and this is why I ask questions.
(cut)
>> Apart from that there are few changes which I don't think are needed
>> and thus are a bit annoying for an author of the code. These are things
>> like changing types of function parameters, renaming the parameters and
>> such. I'd like to see the reasons behind introducing such changes...
>
> IMHO (and this is not an accusation, merely commentary): we as
> developers in a team should never regard any portion of the code higher
> than any other or have moving standards. Ownwership and authorship have
> no place in a modern agile development (like) environment. Arguing over
> parameter names and "why did you add a parameter to my function" are
> controversial and adversarial at best. Let us refrain from this mentality.
>
I strongly disagree here. I am used to open environments where it is always ok
to asks "why?" questions. For example in the scientific world, if you publish
something, you should be always prepared that someone will read your paper
carefully and either criticize it pointing at the errors or (more often) ask you
for explanations. Such behaviour is never considered bad or rude, but it is even
appreciated, because it is one of the main ways of ensuring quality of the
scientific work. I, as an author of scientific papers, am always very happy if
someone asks me for explaining some points and even more happy if he points at
errors in my work because it is the best way for improving it.
I also disagree with "no responsibility for the code" principle. I think not
having a clear responsibilities is one of the things that stops our company from
delivering better results. I'd like to have it very clearly defined and always
know to whom I can turn if I see problems in some parts of the code.
So, I take full responsibility for the code I create and I expect the same from
other developers. And part of this responsibility is to be ready to answer
questions "why have you done it that way". I think it is an obligation of each
developer to be able to answer such questions, especially if asked by reviewer.
For one thing, it can help us to eliminate code which is there for no reason...
Now I even recall some "how to review" document saying that you should not
accept code which you don't understand. And who should explain the code if not
its author?
You should probably also agree that if someone implements feature X, then his
patch should include only changes needed to implement feature X and nothing
more. This is a principle which sometimes can have exceptions (e.g. we fix some
problem or do refactoring in the same patch) but this is an extraordinary
situation and thus it needs a good justification. Changing "const String&"
parameter to "LEX_STRING*" just because I like LEX_STRING more than String is
not a good enough reason...
Ok, that is perhaps enough to explain my position on this. I'm sorry if we
disagree again, but I find it unlikely that I'll change my attitude here.
Rafal
