Skip to main content
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [eclipse-pmc] Review of mass changes 
Hello,

On 6/18/19 5:00 PM, Lars Vogel wrote:

Hi Andrey,

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
negative ones.

To also give an example why I had this impression, here is a quote
from: https://git.eclipse.org/r/c/144388/

````
 From Andrey:
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
previous thread
https://www.eclipse.org/lists/eclipse-pmc/msg03562.html and for
example Danis reply
https://www.eclipse.org/lists/eclipse-pmc/msg03567.html.

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/:
````
 From Lars:
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
welcome
````
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. 


Thanks,
Eric

Back to the top