[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [eclipse-pmc] Review of mass changes
- From: Eric Williams <ericwill@xxxxxxxxxx>
- Date: Wed, 19 Jun 2019 10:42:11 -0400
- Delivered-to: firstname.lastname@example.org
- User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0
On 6/18/19 5:00 PM, Lars Vogel wrote:
thanks for following up on my suggestion, even though you put a
different spin to it.
As I said several times in the Gerrits, my understanding was that your
advice to use empty only for the positive case and size()>0 for the
To also give an example why I had this impression, here is a quote
Lars, I have no clue why do you change perfectly readable .size() > 0
with less readable ! .isEmpty(). Can we please stop this? Especially
there is no one reviewing every line, and as we see there can be
possible bugs coming out of this mass changes.
As I tried to explain, I disagree with the approach to use as mixed
approach of empty/ size in our code and asked you to go to PMC if you
want to change that.
I'm not a PMC member so I don't know what my opinion counts for, but IMO
isEmpty is only readable in the positive case (i.e. to replace getSize()
== 0). Negations before long method/variable names is confusing and not
as readable, especially in if statements. In such cases I would even go
as far to say that the negation should be extracted into an
appropriately named boolean flag, and then used.
Also in our Gerrit discussion I explained to you that the usage of
empty was already addressed before by you to the PMC and that the PMC
decided that empty is better than size. Please see again here, your
https://www.eclipse.org/lists/eclipse-pmc/msg03562.html and for
example Danis reply
I agree with improving readability and I think I reacted to every
single content based feedback you gave. I think your feedback is
valuable and would like to continue to see it.
To give another quote, this time from https://git.eclipse.org/r/c/144392/:
I think you are suggesting to use empty only in the positive case. If
that is your suggestion, yes you have to try convince others that this
is good idea.
If you do code review, than please avoid general statements, which can
be be misunderstood. Feedback to improve these changes is very much
I generally don't mind mass cleanups, but any regressions introduced by
them should be fixed (promptly) by the person doing the mass cleanup, or
the cleanup will be reverted. In SWT we have this unwritten rule and it
works quite well. Also if a change is changing functional code, then
every line should (at the very least) be manually checked.
To the second topic:
I would like to ask PMC here that from now on and for the future such
mass changes that change real code (no white space etc) must have a
mandatory +1 from a second reviewer, to avoid regressions.
We can discuss in the next PMC call, but my personal opinion is that
we should focus our review activities on critical code changes not on
trivial changes. And putting additional review restriction, without
providing additional capacity may be hard to archive.
Maybe we could reach a happy medium: no mass changes/cleanups after M1?
This way it gives maintainers time to test and fix any regressions that
arise. I think having two reviewers mandatory is overkill, but I would
be in favor of some sort of time limitation.