Re: [eclipse-pmc] Review of mass changes

[Lars Vogel <lars.vogel@xxxxxxxxxxx>]

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.

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
````

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.

Best regards, Lars



On Tue, Jun 18, 2019 at 10:07 PM Andrey Loskutov <loskutov@xxxxxx> wrote:
>
> Dear PMC,
>
> First topic.
>
> I was asked by Lars to ask PMC to help with the review of the change
> https://git.eclipse.org/r/#/c/144392/, especially
>
> https://git.eclipse.org/r/#/c/144392/1/bundles/org.eclipse.osgi.compatibility.state/src/org/eclipse/osgi/internal/resolver/StateHelperImpl.java@262
>
> The patch with the title "Use isEmpty instead of size" and the comment
> "Improves readability" suggests to change
>
> return getPossibleCandidates(constraint, exports, null, hook,
> true).size() > 0;
>
> to
>
> return !getPossibleCandidates(constraint, exports, null, hook,
> true).isEmpty();
>
> In my experience such code where "not" is far away from the actual
> negated  method call leads to misinterpretation of the code while
> reading, and therefore does not improve the code readability as claimed
> in the patch comment.
>
> It looks like Lars has different opinion on this topic, so he asked me
> to ask PMC about this.
>
> I personally think that I do not need to ask PMC every time I comment on
> some patch, but if Lars asks, I do, so here we are. I'm sorry for
> wasting your time, but it was not my idea to call PMC.
>
> Please review the patch as asked by Lars.
>
> Second topic.
>
>  From my experience in the past years we had many mass-patches that
> broke something, and I had pleasure to fix them by myself. In most cases
> those mass patches were considered "harmless" and committers never asked
> anybody else for a review and just merged the patches.
>
> Now Lars started another huge patch series that are going across entire
> Eclipse platform, where not just some white space is changed, but where
> the real code (conditionals) are changed, and where very subtle bugs may
> appear. I have no time (and I don't want) to review all those mass
> changes, but someone should, because it is irresponsible to do something
> like this in such way.
>
> 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.
>
> Thanks.
>
> --
> Kind regards,
> Andrey Loskutov
>
> https://www.eclipse.org/user/aloskutov
> ---------------------------------------------
> Спасение утопающих - дело рук самих утопающих
> ---------------------------------------------
> _______________________________________________
> 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 Platform project co-lead
CEO vogella GmbH

Haindaalwisch 17a, 22395 Hamburg
Amtsgericht Hamburg: HRB 127058
Geschäftsführer: Lars Vogel, Jennifer Nerlich de Vogel
USt-IdNr.: DE284122352
Fax (040) 5247 6322, Email: lars.vogel@xxxxxxxxxxx, Web: http://www.vogella.com