Re: [eclipse-pmc] Review of mass changes

["Daniel Megert" <daniel_megert@xxxxxxxxxx>]

>  The
onus should really be on the author to manually verify the mass changes.

I disagree with
that, The author often does his best but might overlook the obvious. As
mentioned, it is OK if only random samples are reviewed. 

Dani



From:
       "Lakshmi
P Shanmugam" <lshanmug@xxxxxxxxxx>
To:
       eclipse-pmc@xxxxxxxxxxx
Date:
       21.06.2019
13:49
Subject:
       [EXTERNAL]
Re: [eclipse-pmc] Review of mass changes
Sent
by:        eclipse-pmc-bounces@xxxxxxxxxxx

---

+1 to not allow mass
changes post M1.
 
Having an additional
reviewer for mass changes is good, but from my own experience it's actually
very time-consuming to review such changes.
The onus should really
be on the author to manually verify the mass changes.
 
Thanks & Regards,
Lakshmi P Shanmugam,
Eclipse Platform Co-lead,
India Software Lab, Bangalore
 
 
----- Original message
-----
From: Eric Williams <ericwill@xxxxxxxxxx>
Sent by: eclipse-pmc-bounces@xxxxxxxxxxx
To: eclipse-pmc@xxxxxxxxxxx
Cc:
Subject: [EXTERNAL] Re: [eclipse-pmc] Review of mass changes
Date: Wed, Jun 19, 2019 8:13 PM
  
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.htmland 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
_______________________________________________
eclipse-pmc mailing list
eclipse-pmc@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe
from this list, visit
https://www.eclipse.org/mailman/listinfo/eclipse-pmc
 
 
_______________________________________________
eclipse-pmc mailing list
eclipse-pmc@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe
from this list, visit
https://www.eclipse.org/mailman/listinfo/eclipse-pmc