Abdelrazak Younes <[EMAIL PROTECTED]> writes:
| > | - With patch oriented review style, it is easier to review a patch
| > | that does only one thing. We are talking here about deep review, here,
| > | not only code sanity review.
| > I look at almost all patches, sure what I most often point at is
| > trivialities, but my main reason for looking at the patch is to learn
| > from it and not be alienated from our sourcebase.
|
| Lars, I am maybe saying something obvious but you really don't have
| the time to do so. I know I have have the time to look at the patches
| related to inset and change-tracking for example. You need to delegate.
I have little enough time as it is, to not completly loose track of what
happens in the source I have to look at patches.
| > | The cons for LyX:
| > | | - We are not students (except for Jose which is a bit late in his
| > | Ph.D. ;-)). I guess most of us have a day job and cannot afford to
| > | spend too much time on LyX. So when one invests some time, he tries to
| > | do as much as he can. Wasting time on patch procedure process is
| > | definitely not fun.
| > I would expect a student to not get the one-change -> one-feature
| > rule, but a professional in the business?
|
| I am a scientific who happens to like C++ development (and who think
| he is quite good at it ;-)). I am not a professional in the software
| business and don't to be. I guess most LyX developers are the same as
| me, aren't they?
Several yes. But at least Andre and I am in the software business.
| I develop LyX for fun, putting professional rules to it makes the
| coding much less attractive to me.
Only if you enter this with the wrong mindset. and I bet just a small
alteration to how to enter this will keep you just as happy as before.
| > If you develop your changes
| > the correct way from the beginning there will be no wasted time on
| > patch procedures.
|
| I disagree... I develop changes my way and I am free to develop anyway
| I like. Your correct way is not _the_ correct way.
But if you do that... you cannot really expect anyone to take your
patches can you? (You have a look on the net on other projects patch
submission policies.)
How you develop and what you develop I have no control over, what we
allow and how we allow it is another matter.
| > But please note: One change might very well be, "Remove GuiWorkarea
| > and GuiCursor out of BufferView."
| > (But if you then also add -> "and change emit to emitSignal" then
| > that
| > is not the case anymore.)
|
| If you really think what you are saying here then you should have said
| it before. For me this "emit" problem was an insignificant detail.
To me that was the showstopper.
| > | - I estimate that only 2 or 3 developers know at the same time a
| > given
| > | area of the source code. This doesn't facilitate patch review.
| > One-change -> one-patch has not much to do with patch review.
|
| So explain what it is all about. And please don't put religious things
| like "we should stick to the rule".
- Simple to do cross-branch merging (or revert)
- a bit same as the principle of "one function one action"
(- review is simpler)
| > (But it helps... instead of making the review process harder (for the
| > reviewr) it mkaes it easier, and thus speeds it up.)
|
| This, I can understand. But did I say already that I hate patch review
| style?
I cannot really control what you hate.
| IMHO, apply the patch and read the code, that's my correct way of
| reviewing a significant change (bug corrections or incremental change
| are not in this case).
So you won't use "My correct way", but we should use yours?
I do not know of any projects that do their work your way. (Only the
ones that do no review at all do it that way.)
| > | - The SVN log is here to explain what is in the commits. Reviewers
| > | that don't know this particular code area just need to verify the C++
| > | correctness and to some extent that what is in the patch corresponds
| > | to what the SVN log says. For reviewers that know this code area, it
| > | should be very easy to review the _contents_ of the patch, even if it
| > | does many things at the same time. Of course some arguing could and
| > | should take place if there is a disagreement.
| > What if I onley know one of the areas that the patch touches, and the
| > other parts is muddled with what I do know and then makes it hard for
| > me?
|
| Sure it could well be that the patch touches many things at the same
| time. In this case, the patch is simply not readable and you really
| should read the resulting code instead. In my case, the "emit" thing
| touched completely separate files so they were not interfering with
| the other changes. So, in my case, this point is void.
| This is actually a good example of what I meant: please don't
| generalize, don't force one rule for all patches just because this
| rule would help for some patches.
eh?
|
| > | Let me take my last patch as an example. This patch did two or
| > three
| > | things but they were all related to the BufferView API cleanup.
| > Except the thing that I really bitched about: emit->emitSignal.
| > That one change make the whole patch unpalatable to me.
|
| You are exaggerating and you know it.
Still that was it.
| > | Conclusion:
| > | | I propose that the LyX community becomes more of a
| > "meritocracy". Once
| > | a developer has acquired enough trust from his fellow LyX developers,
| > | he should be the one who decide what should go in.
| > It is not only about trust. Other minds might have other solutons,
| > perhaps even better ones.
|
| Sure and they are free and welcome to offer their solution. Actually,
| I think I interfered quite a lot in Edwin's and Peter's work and they
| accepted or rejected my advice depending on the case. That's what I
| call collaborative work.
But when I say something you take everything as gospel?
| > | I am not saying
| > | that discussion is forbidden but just that we should not be rigid
| > | about the "rules". Responsible men do not need rules.
| > That sounds almost like a slogan from "NRA"
| > (And thus I disagree.)
|
| Far far from it. We are speaking about a fun project not about
| society.
Sure it is a fun project, but also part of society. There are rules
(also unwritten ones) that we all (try to) follow.
--
Lgb
