[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