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

As agreed by many people on this thread, I would like to reiterate the point that we should not engage in unnecessary cleanups after M1 in any component. Not limited to JDT as we have seen a change in platform https://git.eclipse.org/r/#/c/144099/can cause regression in other dependent components.

I recently see some active cleanup changes in the last development week of M3:
https://git.eclipse.org/r/#/c/147411/
https://git.eclipse.org/r/#/c/144540/
https://git.eclipse.org/r/#/c/147702/

Merge could have been easily postponed to the next release.

Thanks & Regards,
Sarika





From:        "Sarika Sinha" <sarika.sinha@xxxxxxxxxx>
To:        eclipse-pmc@xxxxxxxxxxx
Date:        20/06/2019 09:58 AM
Subject:        [EXTERNAL] Re: [eclipse-pmc] Review of mass changes
Sent by:        eclipse-pmc-bounces@xxxxxxxxxxx




There are few points to be considered -
1. Problem is not with the trivial code changes, problem is in managing the merge conflicts either during this mass change by the contributor or by other contributors after this mass change is released. (As I have seen it happening this week where contributor is struggling to rebase the mass change gerrits)
2. This merging turns to be an evil specially for JDT repositories where new Java version work happens in a different branch (due to legal constraints).


I am OK with the mass changes in M1 but it should be a call of the component based on the timeline and other feature development going on in parallel like JDT.


Thanks & Regards,
Sarika


Inactive hide details for "Daniel Megert" ---06/20/2019 06:01:32 AM---Thanks Mickael, that's a good approach which works fine w"Daniel Megert" ---06/20/2019 06:01:32 AM---Thanks Mickael, that's a good approach which works fine with me. As for the review, yes a second per

From:
"Daniel Megert" <daniel_megert@xxxxxxxxxx>
To:
eclipse-pmc@xxxxxxxxxxx
Date:
06/20/2019 06:01 AM
Subject:
[EXTERNAL] Re: [eclipse-pmc] Review of mass changes
Sent by:
eclipse-pmc-bounces@xxxxxxxxxxx




Thanks Mickael, that's a good approach which works fine with me.

As for the review, yes a second person besides the owner must give a code-review+1 (or+2), but for mass changes with (apparently) trivial code changes I would be OK if only random samples are reviewed.

Dani




From:
Mickael Istria <mistria@xxxxxxxxxx>
To:
"eclipse-pmc@xxxxxxxxxxx" <eclipse-pmc@xxxxxxxxxxx>
Date:
19.06.2019 18:53
Subject:
[EXTERNAL] Re: [eclipse-pmc] Review of mass changes
Sent by:
eclipse-pmc-bounces@xxxxxxxxxxx




Hi,

My 2c below ;)

About !longChain.isEmpty() vs longChain.size() > 0, I favor the first one because isEmpty() is theorically a O(1) operation while size() is a O(n). Of course, most of smart enough implementations have this optimized and make size() a O(1), but there is usually no guarantee it is so. So size() is more expensive that isEmpty() and should be preferred.
About readability, I understand the concern and I would like to suggest an alternative for that case: longChain.isEmpty() == false, which seems to have the qualities requested by all parties.

About requiring a review for mass changes, +1.
About not allowing mass change after some milestone, +1.

Cheers


--
Mickael Istria

Eclipse IDE developer, for Red Hat Developers
_______________________________________________
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

_______________________________________________
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



Back to the top