[
Date Prev][
Date Next][
Thread Prev][
Thread Next][
Date Index][
Thread Index]
[
List Home]
|
Re: [eclipse-pmc] Review of mass changes
|
Hi Andrey,
I'm not on the PMC but I'd like to provide my 2cents.
On Topic 1: I'm 100% with you that the current version is much more
readable
On Topic 2: Once more i can only agree with you
Anyways this is just me, an ordinary (long term) contributor.
Tom
On 18.06.19 22:07, Andrey Loskutov 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
--
Tom Schindl, CTO
BestSolution.at EDV Systemhaus GmbH
Eduard-Bodem-Gasse 5-7. A-6020 Innsbruck
Reg. Nr. FN 222302s am Firmenbuchgericht Innsbruck
