"clean up" again

classic Classic list List threaded Threaded
58 messages Options
123
Reply | Threaded
Open this post in threaded view
|

"clean up" again

stephan.herrmann
Hi,

Another episode in the question whether clean up changes are worth the effort
they cause.

Today the Object Teams build got broken by https://git.eclipse.org/r/#/c/155226/ 
(which doesn't even have a bug that I could re-open).

Object Teams has tons of tests for checking that we don't break JDT. In that
context we have a subclass of org.eclipse.jdt.testplugin.JavaProjectHelper. This
no longer compiles since the above change.

Granted, the package is marked x-internal, so JDT has permission to change any
way we want.

OTOH note that every project that extends JDT is potentially interested in using
also code from the JDT test suite. Here we speak of a fairly large number of
projects.

I would not complain if the change was necessary to implement new functionality
or fix a bug, that's certainly covered by x-internal. But I strongly doubt that
this "clean up" has a benefit that justifies the consequences.

What problem is solved by adding private constructors? Are you doing it just
because it is possible? The commit message doesn't indicate you even thought of
the possibility that s.o. would subclass those classes.

It's too late for changing the code, because I need to fix this today for M3.

But please keep this in mind when doing further clean-up.

Stephan
_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
Reply | Threaded
Open this post in threaded view
|

Re: "clean up" again

Gayan Perera
Hi All,

Good point, But at the same time i think the code change has no issue, i would say it should have made the class final as well looking at the commit message. But i do have another question, why does someone extend  a class which only have static methods ? Isn't that the main issue here ? So i would say the commiter should have made class final as well to make things more clear, and a downstream build should have found this issue sooner if we have such builds on the dependent projects.

Best regards,
Gayan.

On Tue, May 26, 2020 at 8:55 PM Stephan Herrmann <[hidden email]> wrote:
Hi,

Another episode in the question whether clean up changes are worth the effort
they cause.

Today the Object Teams build got broken by https://git.eclipse.org/r/#/c/155226/
(which doesn't even have a bug that I could re-open).

Object Teams has tons of tests for checking that we don't break JDT. In that
context we have a subclass of org.eclipse.jdt.testplugin.JavaProjectHelper. This
no longer compiles since the above change.

Granted, the package is marked x-internal, so JDT has permission to change any
way we want.

OTOH note that every project that extends JDT is potentially interested in using
also code from the JDT test suite. Here we speak of a fairly large number of
projects.

I would not complain if the change was necessary to implement new functionality
or fix a bug, that's certainly covered by x-internal. But I strongly doubt that
this "clean up" has a benefit that justifies the consequences.

What problem is solved by adding private constructors? Are you doing it just
because it is possible? The commit message doesn't indicate you even thought of
the possibility that s.o. would subclass those classes.

It's too late for changing the code, because I need to fix this today for M3.

But please keep this in mind when doing further clean-up.

Stephan
_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev

_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
Reply | Threaded
Open this post in threaded view
|

Re: "clean up" again

Mateusz Matela
In reply to this post by stephan.herrmann
Hi,

I also strongly feel these "cleanups" are not worth it.
I'd love to see a summary of all the bugs introduced by this (maybe a dedicated keyword in bugzilla, if the practice continues?). And they are still being discovered at RC stage, who knows how many will slip through? There's also some extra confusion related to bugs that look like they're probably caused by a "cleanup", but actually are not.

What's the benefit? Less warnings reported by static analysis tools, slightly better readability of the code (most of which may not even be looked at in years).

And yes, one may always argue that in a perfect world this kind of cleaning would not cause any problems, so it's actually revealing preexisting problems. But how relevant are they? A lot of this code has been used for years by plenty of people and nobody cared that some best practices were not applied.

Regards,
Mateusz

wt., 26 maj 2020 o 20:55 Stephan Herrmann <[hidden email]> napisał(a):
Hi,

Another episode in the question whether clean up changes are worth the effort
they cause.

Today the Object Teams build got broken by https://git.eclipse.org/r/#/c/155226/
(which doesn't even have a bug that I could re-open).

Object Teams has tons of tests for checking that we don't break JDT. In that
context we have a subclass of org.eclipse.jdt.testplugin.JavaProjectHelper. This
no longer compiles since the above change.

Granted, the package is marked x-internal, so JDT has permission to change any
way we want.

OTOH note that every project that extends JDT is potentially interested in using
also code from the JDT test suite. Here we speak of a fairly large number of
projects.

I would not complain if the change was necessary to implement new functionality
or fix a bug, that's certainly covered by x-internal. But I strongly doubt that
this "clean up" has a benefit that justifies the consequences.

What problem is solved by adding private constructors? Are you doing it just
because it is possible? The commit message doesn't indicate you even thought of
the possibility that s.o. would subclass those classes.

It's too late for changing the code, because I need to fix this today for M3.

But please keep this in mind when doing further clean-up.

Stephan
_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev

_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
Reply | Threaded
Open this post in threaded view
|

Re: "clean up" again

stephan.herrmann
In reply to this post by Gayan Perera
Gayan, I am not discussing personal preferences of how code should look.

I'm discussing costs vs benefits.

I could put numbers to the costs, but I fail to see substantial benefits (i.e.,
benefits beyond personal taste) in some of the clean ups.

JDT does not live in a vacuum, it has some 20 years of history in the open, and
this implies that this code is being accessed by plenty of code outside JDT. In
this particular case I'm wearing both hats, and it pains me to spend lots of
time on adjusting Object Teams after JDT clean ups, where this time could be
better spent on fixing more bugs.

Saying some class should actually be final is advice coming 20 years late.

If there's one thing to be learned from this: Every change in JDT potentially
affects many other projects.

best
Stephan

On 26.05.20 22:06, Gayan Perera wrote:

> Hi All,
>
> Good point, But at the same time i think the code change has no issue, i would
> say it should have made the class final as well looking at the commit message.
> But i do have another question, why does someone extend  a class which only have
> static methods ? Isn't that the main issue here ? So i would say the
> commiter should have made class final as well to make things more clear, and a
> downstream build should have found this issue sooner if we have such builds on
> the dependent projects.
>
> Best regards,
> Gayan.
>
> On Tue, May 26, 2020 at 8:55 PM Stephan Herrmann <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     Hi,
>
>     Another episode in the question whether clean up changes are worth the effort
>     they cause.
>
>     Today the Object Teams build got broken by
>     https://git.eclipse.org/r/#/c/155226/
>     (which doesn't even have a bug that I could re-open).
>
>     Object Teams has tons of tests for checking that we don't break JDT. In that
>     context we have a subclass of org.eclipse.jdt.testplugin.JavaProjectHelper.
>     This
>     no longer compiles since the above change.
>
>     Granted, the package is marked x-internal, so JDT has permission to change any
>     way we want.
>
>     OTOH note that every project that extends JDT is potentially interested in
>     using
>     also code from the JDT test suite. Here we speak of a fairly large number of
>     projects.
>
>     I would not complain if the change was necessary to implement new functionality
>     or fix a bug, that's certainly covered by x-internal. But I strongly doubt that
>     this "clean up" has a benefit that justifies the consequences.
>
>     What problem is solved by adding private constructors? Are you doing it just
>     because it is possible? The commit message doesn't indicate you even thought of
>     the possibility that s.o. would subclass those classes.
>
>     It's too late for changing the code, because I need to fix this today for M3.
>
>     But please keep this in mind when doing further clean-up.
>
>     Stephan
>     _______________________________________________
>     jdt-dev mailing list
>     [hidden email] <mailto:[hidden email]>
>     To unsubscribe from this list, visit
>     https://www.eclipse.org/mailman/listinfo/jdt-dev
>
>
> _______________________________________________
> jdt-dev mailing list
> [hidden email]
> To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
>

_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
Reply | Threaded
Open this post in threaded view
|

Re: "clean up" again

Mickael Istria-5
In reply to this post by stephan.herrmann
Hi,

I agree that some test utilities are to be considered as APIs.
But if some classes in tests are famous for being used as APIs in downstream projects, why have they been marked as internal for so many years at the risk of seeing such non-backward compatible change happen at some time?
IMO, if some project clearly needs this code as API, then the code should be API and not "x-internal". We have strong capabilities to prevent API breakage, and they can be leveraged easily both in IDE and a buid-time; so there is no technical issue here.
It would be worth reviewing ObjectTeams test and identifying some JDT packages to treat as API and remove the x-internal, to prevent from similar errors in the future.

However, I totally disagree that the code cleanup in particular is the issue here; this change is technically and in terms of governance very correct; it just highlighted some other problems in JDT (non-)APIs and/or downstream project synchronization.
I think the real issue is that with more contributors, the JDT project evolves faster than it used to, and as a consequence, can break things faster for downstream projects that relied on unreliable things.

--
Mickael Istria
Eclipse IDE developer, for Red Hat Developers

_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
Reply | Threaded
Open this post in threaded view
|

Re: "clean up" again

Andrey Loskutov
In reply to this post by Mateusz Matela
I've already expressed my concerns before, but I was told that this is the best way to attract new contributors, and that one should not complain about contributions but be happy that someone touches our bad dirty code.
 
In *my* environment all people that know about this mass changes are only shaking heads about this nonsense. I can't count how many times I've reverted or fixed bugs coming from that, it is a constant pain and constant time killer for me. Backporting or reverting commits without merge issues is almost impossible, because we have numerous commits with gazillion of touched files across the code base. Git history is almost useless now. For one real file change we have 5 various "optimizations" just moving bits around.
 
I personally would understand and accept cleanups only before or after a planned bug fix in the related code.
Cleanups that do *functional* changes (not just white space) shouldn't be done "just because we can".
 
Unfortunately this seem to be a mass sport today and Eclipse platform code is used as a playground for experiments - everyone is welcome to apply "refactoring of the day" on Eclipse code base, "because we can" and because that "attracts new contributors". I doubt that buggy code is attracting and that platform master branch is a good playground for refactorings, but probably I'm alone with this opinion.
 
One explanation (I guess) why some are doing that is that people get job "marketing" for free by pointing on the project stats / git logs and saying "look, I'm Eclipse committer".
This would be OK, if that patches would not harm, and if the people would monitor incoming bugs and fix regressions, but I don't see this happening.
 
Kind regards,
Andrey Loskutov

Спасение утопающих - дело рук самих утопающих

https://www.eclipse.org/user/aloskutov
 
 
Gesendet: Dienstag, 26. Mai 2020 um 23:53 Uhr
Von: "Mateusz Matela" <[hidden email]>
An: "Eclipse JDT general developers list." <[hidden email]>
Betreff: Re: [jdt-dev] "clean up" again
Hi,
 
I also strongly feel these "cleanups" are not worth it.
I'd love to see a summary of all the bugs introduced by this (maybe a dedicated keyword in bugzilla, if the practice continues?). And they are still being discovered at RC stage, who knows how many will slip through? There's also some extra confusion related to bugs that look like they're probably caused by a "cleanup", but actually are not.
 
What's the benefit? Less warnings reported by static analysis tools, slightly better readability of the code (most of which may not even be looked at in years).
 
And yes, one may always argue that in a perfect world this kind of cleaning would not cause any problems, so it's actually revealing preexisting problems. But how relevant are they? A lot of this code has been used for years by plenty of people and nobody cared that some best practices were not applied.
 
Regards,
Mateusz
 
wt., 26 maj 2020 o 20:55 Stephan Herrmann <[hidden email]> napisał(a):
Hi,

Another episode in the question whether clean up changes are worth the effort
they cause.

Today the Object Teams build got broken by https://git.eclipse.org/r/#/c/155226/
(which doesn't even have a bug that I could re-open).

Object Teams has tons of tests for checking that we don't break JDT. In that
context we have a subclass of org.eclipse.jdt.testplugin.JavaProjectHelper. This
no longer compiles since the above change.

Granted, the package is marked x-internal, so JDT has permission to change any
way we want.

OTOH note that every project that extends JDT is potentially interested in using
also code from the JDT test suite. Here we speak of a fairly large number of
projects.

I would not complain if the change was necessary to implement new functionality
or fix a bug, that's certainly covered by x-internal. But I strongly doubt that
this "clean up" has a benefit that justifies the consequences.

What problem is solved by adding private constructors? Are you doing it just
because it is possible? The commit message doesn't indicate you even thought of
the possibility that s.o. would subclass those classes.

It's too late for changing the code, because I need to fix this today for M3.

But please keep this in mind when doing further clean-up.

Stephan
_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
_______________________________________________ jdt-dev mailing list [hidden email] To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev

_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
Reply | Threaded
Open this post in threaded view
|

Re: "clean up" again

Jayaprakash Arthanareeswaran
In reply to this post by Mickael Istria-5
Mickael,

I think you are missing the wood for the trees.

The code in question might well have been an API or an internal code withOUT x-internal but could still have broken something at some point.
That's the problem I see with this kind of mass changes: naturally, it's not possible for every piece of refactored code to get the attention
it deserves. And the fact that this has happened before obviously will make people question the purpose.

Also, there's no point in arguing why things in JDT are in certain way. In a code base that is as old, there are bound to be things that are
less than perfect and people simply don't have the time to fix those or fix the repercussions coming from the mass changes.

Lastly, (I know this has been discussed before, but anyway) a gerrit that touches over a hundred files and 18 patch sets and several failures
in the due course must have its own bug.

Regards,
Jay


[hidden email] wrote: -----
To: "Eclipse JDT general developers list." <[hidden email]>
From: Mickael Istria
Sent by: [hidden email]
Date: 05/27/2020 04:00AM
Subject: [EXTERNAL] Re: [jdt-dev] "clean up" again

Hi,

I agree that some test utilities are to be considered as APIs.
But if some classes in tests are famous for being used as APIs in downstream projects, why have they been marked as internal for so many years at the risk of seeing such non-backward compatible change happen at some time?
IMO, if some project clearly needs this code as API, then the code should be API and not "x-internal". We have strong capabilities to prevent API breakage, and they can be leveraged easily both in IDE and a buid-time; so there is no technical issue here.
It would be worth reviewing ObjectTeams test and identifying some JDT packages to treat as API and remove the x-internal, to prevent from similar errors in the future.

However, I totally disagree that the code cleanup in particular is the issue here; this change is technically and in terms of governance very correct; it just highlighted some other problems in JDT (non-)APIs and/or downstream project synchronization.
I think the real issue is that with more contributors, the JDT project evolves faster than it used to, and as a consequence, can break things faster for downstream projects that relied on unreliable things.

--
Mickael Istria
Eclipse IDE developer, for Red Hat Developers
_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev


_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
Reply | Threaded
Open this post in threaded view
|

Re: "clean up" again

Aleksandar Kurtakov
In reply to this post by Andrey Loskutov


On Wed, May 27, 2020 at 1:30 AM Andrey Loskutov <[hidden email]> wrote:
I've already expressed my concerns before, but I was told that this is


the best way to attract new contributors

I fully stand behind ^^ - just look at the git history and how much Fabrice, Kenneth and Carsten (sorry if I've missed someone improved editing experience, cleanups and save actions in the last couple of releases. Both in terms of bug fixes to existing ones (like not losing code comments when applying a cleanup) and adding new such (whether we like it or not new users measure us by trying things like https://twitter.com/ZhekaKozlov/status/1135506701438857217 in Eclipse).
If we fail at attracting new users and contributors there would be more and more work on us (current committers) as the changes in environment only accelerate.
With the above said  - I fully understand this causes issues and that the duty to fix these ends up on current committers. I can definitely put myself in both positions - I still remember the endless crash story with SWT when distributions removed gnome-vfs (when and why I started contributing to SWT) and the fact that SWT codebase was just convoluted spaghetti plate so in order to fix a single issue I ended up changing test suites so they run locally, make it use Java 5 features, drop workarounds for Java 1.1 or J2ME and etc. and only after doing that I was able to fix a simple cut-off drawing of a text field. The reasons for that are 2 - most developers feel extremely discouraged when they are thrown into an unknown and ancient codebase and second by doing these "cleanups" one gains a significantly better understanding of the codebase.
I have introduced numerous issues by doing that but Silenio (mostly) and Bogdan handholded me and talked to me to help resolve the issues to the state when I was in their shoes and doing the same with Anatoly, Leo, Eric, Xi - the group of people that helped stabilize SWT (on GTK) to the point where things are still running OK even after they moved on to work on other things.
But people move on and we have to attract new contributors. This is the way we (people actively recruiting new contributors) know and the way we do it.
I would be more than happy if someone proves me wrong and manages to get the benefits without the (temporary) drawbacks.


 
, and that one should not complain about contributions but be happy that someone touches our bad dirty code.
 
In *my* environment all people that know about this mass changes are only shaking heads about this nonsense. I can't count how many times I've reverted or fixed bugs coming from that, it is a constant pain and constant time killer for me. Backporting or reverting commits without merge issues is almost impossible, because we have numerous commits with gazillion of touched files across the code base. Git history is almost useless now. For one real file change we have 5 various "optimizations" just moving bits around.
 
I personally would understand and accept cleanups only before or after a planned bug fix in the related code.
Cleanups that do *functional* changes (not just white space) shouldn't be done "just because we can".
 
Unfortunately this seem to be a mass sport today and Eclipse platform code is used as a playground for experiments - everyone is welcome to apply "refactoring of the day" on Eclipse code base, "because we can" and because that "attracts new contributors". I doubt that buggy code is attracting and that platform master branch is a good playground for refactorings, but probably I'm alone with this opinion.
 
One explanation (I guess) why some are doing that is that people get job "marketing" for free by pointing on the project stats / git logs and saying "look, I'm Eclipse committer".
This would be OK, if that patches would not harm, and if the people would monitor incoming bugs and fix regressions, but I don't see this happening.
 
Kind regards,
Andrey Loskutov

Спасение утопающих - дело рук самих утопающих

https://www.eclipse.org/user/aloskutov
 
 
Gesendet: Dienstag, 26. Mai 2020 um 23:53 Uhr
Von: "Mateusz Matela" <[hidden email]>
An: "Eclipse JDT general developers list." <[hidden email]>
Betreff: Re: [jdt-dev] "clean up" again
Hi,
 
I also strongly feel these "cleanups" are not worth it.
I'd love to see a summary of all the bugs introduced by this (maybe a dedicated keyword in bugzilla, if the practice continues?). And they are still being discovered at RC stage, who knows how many will slip through? There's also some extra confusion related to bugs that look like they're probably caused by a "cleanup", but actually are not.
 
What's the benefit? Less warnings reported by static analysis tools, slightly better readability of the code (most of which may not even be looked at in years).
 
And yes, one may always argue that in a perfect world this kind of cleaning would not cause any problems, so it's actually revealing preexisting problems. But how relevant are they? A lot of this code has been used for years by plenty of people and nobody cared that some best practices were not applied.
 
Regards,
Mateusz
 
wt., 26 maj 2020 o 20:55 Stephan Herrmann <[hidden email]> napisał(a):
Hi,

Another episode in the question whether clean up changes are worth the effort
they cause.

Today the Object Teams build got broken by https://git.eclipse.org/r/#/c/155226/
(which doesn't even have a bug that I could re-open).

Object Teams has tons of tests for checking that we don't break JDT. In that
context we have a subclass of org.eclipse.jdt.testplugin.JavaProjectHelper. This
no longer compiles since the above change.

Granted, the package is marked x-internal, so JDT has permission to change any
way we want.

OTOH note that every project that extends JDT is potentially interested in using
also code from the JDT test suite. Here we speak of a fairly large number of
projects.

I would not complain if the change was necessary to implement new functionality
or fix a bug, that's certainly covered by x-internal. But I strongly doubt that
this "clean up" has a benefit that justifies the consequences.

What problem is solved by adding private constructors? Are you doing it just
because it is possible? The commit message doesn't indicate you even thought of
the possibility that s.o. would subclass those classes.

It's too late for changing the code, because I need to fix this today for M3.

But please keep this in mind when doing further clean-up.

Stephan
_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
_______________________________________________ jdt-dev mailing list [hidden email] To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev


--
Alexander Kurtakov
Red Hat Eclipse Team

_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
Reply | Threaded
Open this post in threaded view
|

Re: "clean up" again

Fabrice Tiercelin
In reply to this post by stephan.herrmann
Hi,

I prefer not arguing today in this tense situation. I will argue later in a more peaceful context.

Regards,
Fabrice TIERCELIN

Le mardi 26 mai 2020 à 20:55:48 UTC+2, Stephan Herrmann <[hidden email]> a écrit :


Hi,

Another episode in the question whether clean up changes are worth the effort
they cause.

Today the Object Teams build got broken by https://git.eclipse.org/r/#/c/155226/
(which doesn't even have a bug that I could re-open).

Object Teams has tons of tests for checking that we don't break JDT. In that
context we have a subclass of org.eclipse.jdt.testplugin.JavaProjectHelper. This
no longer compiles since the above change.

Granted, the package is marked x-internal, so JDT has permission to change any
way we want.

OTOH note that every project that extends JDT is potentially interested in using
also code from the JDT test suite. Here we speak of a fairly large number of
projects.

I would not complain if the change was necessary to implement new functionality
or fix a bug, that's certainly covered by x-internal. But I strongly doubt that
this "clean up" has a benefit that justifies the consequences.

What problem is solved by adding private constructors? Are you doing it just
because it is possible? The commit message doesn't indicate you even thought of
the possibility that s.o. would subclass those classes.

It's too late for changing the code, because I need to fix this today for M3.

But please keep this in mind when doing further clean-up.

Stephan
_______________________________________________
jdt-dev mailing list
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev

_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
Reply | Threaded
Open this post in threaded view
|

Re: "clean up" again

Till Brychcy
In reply to this post by Mickael Istria-5


> Am 27.05.2020 um 00:29 schrieb Mickael Istria <[hidden email]>:
> But if some classes in tests are famous for being used as APIs in downstream projects, why have they been marked as internal for so many years at the risk of seeing such non-backward compatible change happen at some time?
> IMO, if some project clearly needs this code as API, then the code should be API and not "x-internal". We have strong capabilities to prevent API breakage, and they can be leveraged easily both in IDE and a buid-time; so there is no technical issue here.

That may be true. I even created a few tickets for such code:

https://bugs.eclipse.org/bugs/show_bug.cgi?id=527754
https://bugs.eclipse.org/bugs/show_bug.cgi?id=527756

But it is a lot of work and nobody works on them (and I don’t want to either, as all contributions are in my spare time), so I stopped creating tickets like this after the auto-closing startet.

Also note the number of "x-friends“-declaration in the MANIFEST.MF-File of Platform and JDT. Almost all of them point to code that should probably be API.



My take on "cleanups“:

The conversion to enhanced „for“-loops was the only one that in my eyes is actually is purely a improvement, often they are just style changes.
- A lambda is at least almost always better than an anonymous class, but also not always
- A method reference is not always better than a lambda
- A switch is not always better than a if-cascade
- A pushed down negation is not always easiertto read
...

So I think that most of these „cleanups“ should be done on when somebody works on the code and can make the proper decisions and not by running some automated tool.

And even then, there should be a bug for all substantial changes (not trivial stuff like doc fixes):

First of all, it is easier to discuss and reference later.
But almost as important, at least I only watch activity in bugzilla, so I only see these changes without bugs AFTER they are commited, so they are always a surprise, and then, hard to discuss afterwards.

And no, I think it is not reasonable to expect that gerrit is monitored additionally.

(BTW: There are some very valuable „cleanups" that would be a great improvement:
- add type parameters to raw types
- add null type annotations.
Of this would be a lot of work, so nobody works on this.)




_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
Reply | Threaded
Open this post in threaded view
|

Re: "clean up" again

stephan.herrmann
In reply to this post by stephan.herrmann

In my post I mainly wanted to raise awareness that JDT code (even if x-internal) is potentially consumed outside JDT, and that even seemingly trivial changes can (and do) cause havoc downstream.

Now that the discussion has been broadened to the general issue of clean ups, I would like to list three kinds of clean-ups that I do consider useful:

1. Refactorings that help fixing a bug. This could be (a) a refactoring as part of the process of understanding some old code section, or (b) a refactoring that prepares for the desired solution.

2. Changes that improve the ability to detect potential bugs using JDT's own analysis, like avoiding raw types, adding null annotations (careful when API is affected!).

3. Refactorings that are performed for the purpose of testing our own functionality in a dog-fooding like approach.


I suggest that (1) and (2) are encouraged on our productive code base, and that branches are created for experiments in (3). These branches can be made available for voluntary field testing but should not be merged to master.

Types (1) and (2) need a bugzilla for every change.

If (3) is performed on a branch, perhaps one umbrella bug can cover several experiments.

best,
Stephan


Am 2020-05-26 20:55, schrieb Stephan Herrmann:

Hi,

Another episode in the question whether clean up changes are worth the effort they cause.

Today the Object Teams build got broken by https://git.eclipse.org/r/#/c/155226/ (which doesn't even have a bug that I could re-open).

Object Teams has tons of tests for checking that we don't break JDT. In that context we have a subclass of org.eclipse.jdt.testplugin.JavaProjectHelper. This no longer compiles since the above change.

Granted, the package is marked x-internal, so JDT has permission to change any way we want.

OTOH note that every project that extends JDT is potentially interested in using also code from the JDT test suite. Here we speak of a fairly large number of projects.

I would not complain if the change was necessary to implement new functionality or fix a bug, that's certainly covered by x-internal. But I strongly doubt that this "clean up" has a benefit that justifies the consequences.

What problem is solved by adding private constructors? Are you doing it just because it is possible? The commit message doesn't indicate you even thought of the possibility that s.o. would subclass those classes.

It's too late for changing the code, because I need to fix this today for M3.

But please keep this in mind when doing further clean-up.

Stephan
_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev



_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
Reply | Threaded
Open this post in threaded view
|

Re: "clean up" again

Lars Vogel-2
I agree with Alex and the position of the PMC. To attract new
contributors and potential committers, we encourage cleanup patches.

This strategy seems to work in JDT UI, lots of the bugs fixed in 4.16
were done by "new" contributors / committers.

See https://bugs.eclipse.org/bugs/buglist.cgi?bug_status=RESOLVED&bug_status=VERIFIED&classification=Eclipse%20Project&component=UI&email1=Lars.Vogel%40vogella.com&emailtype1=substring&known_name=Eclipse%20Current%20Milestone&list_id=19563803&product=JDT&query_based_on=Eclipse%20Current%20Milestone&query_format=advanced&resolution=FIXED&target_milestone=4.16&target_milestone=4.16%20M1&target_milestone=4.16%20M3&target_milestone=4.16%20RC1&target_milestone=4.16%20RC2

As an end user of JDT I benefit from this rush of new functionality
and as a PMC member I'm very happy to see so many improvements in JDT
from existing and new committers and contributors.

Best regards, Lars

On Wed, May 27, 2020 at 1:10 PM <[hidden email]> wrote:

>
> In my post I mainly wanted to raise awareness that JDT code (even if x-internal) is potentially consumed outside JDT, and that even seemingly trivial changes can (and do) cause havoc downstream.
>
> Now that the discussion has been broadened to the general issue of clean ups, I would like to list three kinds of clean-ups that I do consider useful:
>
> 1. Refactorings that help fixing a bug. This could be (a) a refactoring as part of the process of understanding some old code section, or (b) a refactoring that prepares for the desired solution.
>
> 2. Changes that improve the ability to detect potential bugs using JDT's own analysis, like avoiding raw types, adding null annotations (careful when API is affected!).
>
> 3. Refactorings that are performed for the purpose of testing our own functionality in a dog-fooding like approach.
>
>
> I suggest that (1) and (2) are encouraged on our productive code base, and that branches are created for experiments in (3). These branches can be made available for voluntary field testing but should not be merged to master.
>
> Types (1) and (2) need a bugzilla for every change.
>
> If (3) is performed on a branch, perhaps one umbrella bug can cover several experiments.
>
> best,
> Stephan
>
>
> Am 2020-05-26 20:55, schrieb Stephan Herrmann:
>
> Hi,
>
> Another episode in the question whether clean up changes are worth the effort they cause.
>
> Today the Object Teams build got broken by https://git.eclipse.org/r/#/c/155226/ (which doesn't even have a bug that I could re-open).
>
> Object Teams has tons of tests for checking that we don't break JDT. In that context we have a subclass of org.eclipse.jdt.testplugin.JavaProjectHelper. This no longer compiles since the above change.
>
> Granted, the package is marked x-internal, so JDT has permission to change any way we want.
>
> OTOH note that every project that extends JDT is potentially interested in using also code from the JDT test suite. Here we speak of a fairly large number of projects.
>
> I would not complain if the change was necessary to implement new functionality or fix a bug, that's certainly covered by x-internal. But I strongly doubt that this "clean up" has a benefit that justifies the consequences.
>
> What problem is solved by adding private constructors? Are you doing it just because it is possible? The commit message doesn't indicate you even thought of the possibility that s.o. would subclass those classes.
>
> It's too late for changing the code, because I need to fix this today for M3.
>
> But please keep this in mind when doing further clean-up.
>
> Stephan
> _______________________________________________
> jdt-dev mailing list
> [hidden email]
> To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
>
>
> _______________________________________________
> jdt-dev mailing list
> [hidden email]
> To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev



--
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: [hidden email], Web: http://www.vogella.com
_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
Reply | Threaded
Open this post in threaded view
|

Re: "clean up" again

Lars Vogel-2
In reply to this post by stephan.herrmann
Stephan, you mentioned using null annotation. Does this work these
days for our code? Is JDT using it?

We tried in platform to use it a few times but we failed in the past,
see for example: https://bugs.eclipse.org/bugs/show_bug.cgi?id=472631

On Wed, May 27, 2020 at 1:10 PM <[hidden email]> wrote:

>
> In my post I mainly wanted to raise awareness that JDT code (even if x-internal) is potentially consumed outside JDT, and that even seemingly trivial changes can (and do) cause havoc downstream.
>
> Now that the discussion has been broadened to the general issue of clean ups, I would like to list three kinds of clean-ups that I do consider useful:
>
> 1. Refactorings that help fixing a bug. This could be (a) a refactoring as part of the process of understanding some old code section, or (b) a refactoring that prepares for the desired solution.
>
> 2. Changes that improve the ability to detect potential bugs using JDT's own analysis, like avoiding raw types, adding null annotations (careful when API is affected!).
>
> 3. Refactorings that are performed for the purpose of testing our own functionality in a dog-fooding like approach.
>
>
> I suggest that (1) and (2) are encouraged on our productive code base, and that branches are created for experiments in (3). These branches can be made available for voluntary field testing but should not be merged to master.
>
> Types (1) and (2) need a bugzilla for every change.
>
> If (3) is performed on a branch, perhaps one umbrella bug can cover several experiments.
>
> best,
> Stephan
>
>
> Am 2020-05-26 20:55, schrieb Stephan Herrmann:
>
> Hi,
>
> Another episode in the question whether clean up changes are worth the effort they cause.
>
> Today the Object Teams build got broken by https://git.eclipse.org/r/#/c/155226/ (which doesn't even have a bug that I could re-open).
>
> Object Teams has tons of tests for checking that we don't break JDT. In that context we have a subclass of org.eclipse.jdt.testplugin.JavaProjectHelper. This no longer compiles since the above change.
>
> Granted, the package is marked x-internal, so JDT has permission to change any way we want.
>
> OTOH note that every project that extends JDT is potentially interested in using also code from the JDT test suite. Here we speak of a fairly large number of projects.
>
> I would not complain if the change was necessary to implement new functionality or fix a bug, that's certainly covered by x-internal. But I strongly doubt that this "clean up" has a benefit that justifies the consequences.
>
> What problem is solved by adding private constructors? Are you doing it just because it is possible? The commit message doesn't indicate you even thought of the possibility that s.o. would subclass those classes.
>
> It's too late for changing the code, because I need to fix this today for M3.
>
> But please keep this in mind when doing further clean-up.
>
> Stephan
> _______________________________________________
> jdt-dev mailing list
> [hidden email]
> To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
>
>
> _______________________________________________
> jdt-dev mailing list
> [hidden email]
> To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev



--
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: [hidden email], Web: http://www.vogella.com
_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
Reply | Threaded
Open this post in threaded view
|

Re: "clean up" again

stephan.herrmann
In reply to this post by stephan.herrmann

Still one more suggestion / request:

Let's please discuss this JDT issue as a JDT issue only.

Platform is different. Hence also the p.o.v. of the Eclipse PMC is different from the day-to-day work in JDT.

Let's find out what's best for JDT.

thanks,
Stephan

Am 2020-05-27 13:10, schrieb [hidden email]:

In my post I mainly wanted to raise awareness that JDT code (even if x-internal) is potentially consumed outside JDT, and that even seemingly trivial changes can (and do) cause havoc downstream.

Now that the discussion has been broadened to the general issue of clean ups, I would like to list three kinds of clean-ups that I do consider useful:

1. Refactorings that help fixing a bug. This could be (a) a refactoring as part of the process of understanding some old code section, or (b) a refactoring that prepares for the desired solution.

2. Changes that improve the ability to detect potential bugs using JDT's own analysis, like avoiding raw types, adding null annotations (careful when API is affected!).

3. Refactorings that are performed for the purpose of testing our own functionality in a dog-fooding like approach.


I suggest that (1) and (2) are encouraged on our productive code base, and that branches are created for experiments in (3). These branches can be made available for voluntary field testing but should not be merged to master.

Types (1) and (2) need a bugzilla for every change.

If (3) is performed on a branch, perhaps one umbrella bug can cover several experiments.

best,
Stephan


Am 2020-05-26 20:55, schrieb Stephan Herrmann:

Hi,

Another episode in the question whether clean up changes are worth the effort they cause.

Today the Object Teams build got broken by https://git.eclipse.org/r/#/c/155226/ (which doesn't even have a bug that I could re-open).

Object Teams has tons of tests for checking that we don't break JDT. In that context we have a subclass of org.eclipse.jdt.testplugin.JavaProjectHelper. This no longer compiles since the above change.

Granted, the package is marked x-internal, so JDT has permission to change any way we want.

OTOH note that every project that extends JDT is potentially interested in using also code from the JDT test suite. Here we speak of a fairly large number of projects.

I would not complain if the change was necessary to implement new functionality or fix a bug, that's certainly covered by x-internal. But I strongly doubt that this "clean up" has a benefit that justifies the consequences.

What problem is solved by adding private constructors? Are you doing it just because it is possible? The commit message doesn't indicate you even thought of the possibility that s.o. would subclass those classes.

It's too late for changing the code, because I need to fix this today for M3.

But please keep this in mind when doing further clean-up.

Stephan
_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev



_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev



_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
Reply | Threaded
Open this post in threaded view
|

Re: "clean up" again

stephan.herrmann

To really get the full picture, I would very much like to hear from our new contributors, how they see the connection between clean up changes and functional changes / bug fixes.

Is there any connection or are these disjoint activities?

If connected, how exactly?

thanks,
Stephan

Am 2020-05-27 16:21, schrieb [hidden email]:

Still one more suggestion / request:

Let's please discuss this JDT issue as a JDT issue only.

Platform is different. Hence also the p.o.v. of the Eclipse PMC is different from the day-to-day work in JDT.

Let's find out what's best for JDT.

thanks,
Stephan

Am 2020-05-27 13:10, schrieb [hidden email]:

In my post I mainly wanted to raise awareness that JDT code (even if x-internal) is potentially consumed outside JDT, and that even seemingly trivial changes can (and do) cause havoc downstream.

Now that the discussion has been broadened to the general issue of clean ups, I would like to list three kinds of clean-ups that I do consider useful:

1. Refactorings that help fixing a bug. This could be (a) a refactoring as part of the process of understanding some old code section, or (b) a refactoring that prepares for the desired solution.

2. Changes that improve the ability to detect potential bugs using JDT's own analysis, like avoiding raw types, adding null annotations (careful when API is affected!).

3. Refactorings that are performed for the purpose of testing our own functionality in a dog-fooding like approach.


I suggest that (1) and (2) are encouraged on our productive code base, and that branches are created for experiments in (3). These branches can be made available for voluntary field testing but should not be merged to master.

Types (1) and (2) need a bugzilla for every change.

If (3) is performed on a branch, perhaps one umbrella bug can cover several experiments.

best,
Stephan


Am 2020-05-26 20:55, schrieb Stephan Herrmann:

Hi,

Another episode in the question whether clean up changes are worth the effort they cause.

Today the Object Teams build got broken by https://git.eclipse.org/r/#/c/155226/ (which doesn't even have a bug that I could re-open).

Object Teams has tons of tests for checking that we don't break JDT. In that context we have a subclass of org.eclipse.jdt.testplugin.JavaProjectHelper. This no longer compiles since the above change.

Granted, the package is marked x-internal, so JDT has permission to change any way we want.

OTOH note that every project that extends JDT is potentially interested in using also code from the JDT test suite. Here we speak of a fairly large number of projects.

I would not complain if the change was necessary to implement new functionality or fix a bug, that's certainly covered by x-internal. But I strongly doubt that this "clean up" has a benefit that justifies the consequences.

What problem is solved by adding private constructors? Are you doing it just because it is possible? The commit message doesn't indicate you even thought of the possibility that s.o. would subclass those classes.

It's too late for changing the code, because I need to fix this today for M3.

But please keep this in mind when doing further clean-up.

Stephan
_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev



_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev



_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev



_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
Reply | Threaded
Open this post in threaded view
|

Re: "clean up" again

Kenneth Styrberg-2

I see them as disjointed activity. I think fixing a bug shouldn't cover clean-up/re-factoring of the classes, this makes reviewing the bugfix much harder. This is of course up to the committer, if the file is small maybe a full cleanup is possible without making reviewing too difficult.

Regards,

Kenneth

Den 2020-05-27 kl. 16:40, skrev [hidden email]:

To really get the full picture, I would very much like to hear from our new contributors, how they see the connection between clean up changes and functional changes / bug fixes.

Is there any connection or are these disjoint activities?

If connected, how exactly?

thanks,
Stephan

Am 2020-05-27 16:21, schrieb [hidden email]:

Still one more suggestion / request:

Let's please discuss this JDT issue as a JDT issue only.

Platform is different. Hence also the p.o.v. of the Eclipse PMC is different from the day-to-day work in JDT.

Let's find out what's best for JDT.

thanks,
Stephan

Am 2020-05-27 13:10, schrieb [hidden email]:

In my post I mainly wanted to raise awareness that JDT code (even if x-internal) is potentially consumed outside JDT, and that even seemingly trivial changes can (and do) cause havoc downstream.

Now that the discussion has been broadened to the general issue of clean ups, I would like to list three kinds of clean-ups that I do consider useful:

1. Refactorings that help fixing a bug. This could be (a) a refactoring as part of the process of understanding some old code section, or (b) a refactoring that prepares for the desired solution.

2. Changes that improve the ability to detect potential bugs using JDT's own analysis, like avoiding raw types, adding null annotations (careful when API is affected!).

3. Refactorings that are performed for the purpose of testing our own functionality in a dog-fooding like approach.


I suggest that (1) and (2) are encouraged on our productive code base, and that branches are created for experiments in (3). These branches can be made available for voluntary field testing but should not be merged to master.

Types (1) and (2) need a bugzilla for every change.

If (3) is performed on a branch, perhaps one umbrella bug can cover several experiments.

best,
Stephan


Am 2020-05-26 20:55, schrieb Stephan Herrmann:

Hi,

Another episode in the question whether clean up changes are worth the effort they cause.

Today the Object Teams build got broken by https://git.eclipse.org/r/#/c/155226/ (which doesn't even have a bug that I could re-open).

Object Teams has tons of tests for checking that we don't break JDT. In that context we have a subclass of org.eclipse.jdt.testplugin.JavaProjectHelper. This no longer compiles since the above change.

Granted, the package is marked x-internal, so JDT has permission to change any way we want.

OTOH note that every project that extends JDT is potentially interested in using also code from the JDT test suite. Here we speak of a fairly large number of projects.

I would not complain if the change was necessary to implement new functionality or fix a bug, that's certainly covered by x-internal. But I strongly doubt that this "clean up" has a benefit that justifies the consequences.

What problem is solved by adding private constructors? Are you doing it just because it is possible? The commit message doesn't indicate you even thought of the possibility that s.o. would subclass those classes.

It's too late for changing the code, because I need to fix this today for M3.

But please keep this in mind when doing further clean-up.

Stephan
_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev



_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev



_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev



_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev

_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
Reply | Threaded
Open this post in threaded view
|

Re: "clean up" again

stephan.herrmann
In reply to this post by stephan.herrmann

Anything in the order of "attracting contributors"?

Is it more attractive to start working on a component by making clean up changes? Do clean up changes help to get involved and serve as motivation to start working on functional changes, too?

Have you observed difficulties in understanding JDT code, that were resolved by doing a clean up change first? Examples?

thanks,
Stephan

Am 2020-05-27 16:40, schrieb [hidden email]:

To really get the full picture, I would very much like to hear from our new contributors, how they see the connection between clean up changes and functional changes / bug fixes.

Is there any connection or are these disjoint activities?

If connected, how exactly?

thanks,
Stephan

Am 2020-05-27 16:21, schrieb [hidden email]:

Still one more suggestion / request:

Let's please discuss this JDT issue as a JDT issue only.

Platform is different. Hence also the p.o.v. of the Eclipse PMC is different from the day-to-day work in JDT.

Let's find out what's best for JDT.

thanks,
Stephan

Am 2020-05-27 13:10, schrieb [hidden email]:

In my post I mainly wanted to raise awareness that JDT code (even if x-internal) is potentially consumed outside JDT, and that even seemingly trivial changes can (and do) cause havoc downstream.

Now that the discussion has been broadened to the general issue of clean ups, I would like to list three kinds of clean-ups that I do consider useful:

1. Refactorings that help fixing a bug. This could be (a) a refactoring as part of the process of understanding some old code section, or (b) a refactoring that prepares for the desired solution.

2. Changes that improve the ability to detect potential bugs using JDT's own analysis, like avoiding raw types, adding null annotations (careful when API is affected!).

3. Refactorings that are performed for the purpose of testing our own functionality in a dog-fooding like approach.


I suggest that (1) and (2) are encouraged on our productive code base, and that branches are created for experiments in (3). These branches can be made available for voluntary field testing but should not be merged to master.

Types (1) and (2) need a bugzilla for every change.

If (3) is performed on a branch, perhaps one umbrella bug can cover several experiments.

best,
Stephan


Am 2020-05-26 20:55, schrieb Stephan Herrmann:

Hi,

Another episode in the question whether clean up changes are worth the effort they cause.

Today the Object Teams build got broken by https://git.eclipse.org/r/#/c/155226/ (which doesn't even have a bug that I could re-open).

Object Teams has tons of tests for checking that we don't break JDT. In that context we have a subclass of org.eclipse.jdt.testplugin.JavaProjectHelper. This no longer compiles since the above change.

Granted, the package is marked x-internal, so JDT has permission to change any way we want.

OTOH note that every project that extends JDT is potentially interested in using also code from the JDT test suite. Here we speak of a fairly large number of projects.

I would not complain if the change was necessary to implement new functionality or fix a bug, that's certainly covered by x-internal. But I strongly doubt that this "clean up" has a benefit that justifies the consequences.

What problem is solved by adding private constructors? Are you doing it just because it is possible? The commit message doesn't indicate you even thought of the possibility that s.o. would subclass those classes.

It's too late for changing the code, because I need to fix this today for M3.

But please keep this in mind when doing further clean-up.

Stephan
_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev



_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev



_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev



_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev



_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
Reply | Threaded
Open this post in threaded view
|

Re: "clean up" again

Kenneth Styrberg-2

The main point for me was to fix bugs. At first when I did my first patches I also removed all warnings from the code, but was encouraged not to do so to simplify reviews.

To do clean-ups as a starting point wasn't a thing for me.  Maybe a more junior programmer finds that more gratifying than me. My main motivation to continue was a responsive committer that actually took the time a review the patches and came with constructive comments. Even just a comment that he/she will look into it later, made it feel good, and that my time wasn't wasted.

I think doing clean-ups doesn't help ju understand the JDT code, you just follow a pattern without the need to know what the code actually do. Sure you learn how to setup your IDE connect to GIT and commit changes to Gerrit, that was also a blocker for which I had to reach out for help to solve, so it might be a first entry for some.

Regards,

Kenneth

Den 2020-05-27 kl. 19:46, skrev [hidden email]:

Anything in the order of "attracting contributors"?

Is it more attractive to start working on a component by making clean up changes? Do clean up changes help to get involved and serve as motivation to start working on functional changes, too?

Have you observed difficulties in understanding JDT code, that were resolved by doing a clean up change first? Examples?

thanks,
Stephan

Am 2020-05-27 16:40, schrieb [hidden email]:

To really get the full picture, I would very much like to hear from our new contributors, how they see the connection between clean up changes and functional changes / bug fixes.

Is there any connection or are these disjoint activities?

If connected, how exactly?

thanks,
Stephan

Am 2020-05-27 16:21, schrieb [hidden email]:

Still one more suggestion / request:

Let's please discuss this JDT issue as a JDT issue only.

Platform is different. Hence also the p.o.v. of the Eclipse PMC is different from the day-to-day work in JDT.

Let's find out what's best for JDT.

thanks,
Stephan

Am 2020-05-27 13:10, schrieb [hidden email]:

In my post I mainly wanted to raise awareness that JDT code (even if x-internal) is potentially consumed outside JDT, and that even seemingly trivial changes can (and do) cause havoc downstream.

Now that the discussion has been broadened to the general issue of clean ups, I would like to list three kinds of clean-ups that I do consider useful:

1. Refactorings that help fixing a bug. This could be (a) a refactoring as part of the process of understanding some old code section, or (b) a refactoring that prepares for the desired solution.

2. Changes that improve the ability to detect potential bugs using JDT's own analysis, like avoiding raw types, adding null annotations (careful when API is affected!).

3. Refactorings that are performed for the purpose of testing our own functionality in a dog-fooding like approach.


I suggest that (1) and (2) are encouraged on our productive code base, and that branches are created for experiments in (3). These branches can be made available for voluntary field testing but should not be merged to master.

Types (1) and (2) need a bugzilla for every change.

If (3) is performed on a branch, perhaps one umbrella bug can cover several experiments.

best,
Stephan


Am 2020-05-26 20:55, schrieb Stephan Herrmann:

Hi,

Another episode in the question whether clean up changes are worth the effort they cause.

Today the Object Teams build got broken by https://git.eclipse.org/r/#/c/155226/ (which doesn't even have a bug that I could re-open).

Object Teams has tons of tests for checking that we don't break JDT. In that context we have a subclass of org.eclipse.jdt.testplugin.JavaProjectHelper. This no longer compiles since the above change.

Granted, the package is marked x-internal, so JDT has permission to change any way we want.

OTOH note that every project that extends JDT is potentially interested in using also code from the JDT test suite. Here we speak of a fairly large number of projects.

I would not complain if the change was necessary to implement new functionality or fix a bug, that's certainly covered by x-internal. But I strongly doubt that this "clean up" has a benefit that justifies the consequences.

What problem is solved by adding private constructors? Are you doing it just because it is possible? The commit message doesn't indicate you even thought of the possibility that s.o. would subclass those classes.

It's too late for changing the code, because I need to fix this today for M3.

But please keep this in mind when doing further clean-up.

Stephan
_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev



_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev



_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev



_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev



_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev

_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
Reply | Threaded
Open this post in threaded view
|

Re: "clean up" again

Jeff Johnston
Not me personally, but I have been reviewing a number of cleanup changes since I started as a JDT committer.  Carsten Hammer has contributed a large number of these.
That said, I have noticed that Carsten has started to test and is opening bugs, creating test cases, and has recently started providing the odd patch so he is
gradually dipping his toes into contributing more than just cleanups.

-- Jeff J.

On Wed, May 27, 2020 at 2:25 PM Kenneth Styrberg <[hidden email]> wrote:

The main point for me was to fix bugs. At first when I did my first patches I also removed all warnings from the code, but was encouraged not to do so to simplify reviews.

To do clean-ups as a starting point wasn't a thing for me.  Maybe a more junior programmer finds that more gratifying than me. My main motivation to continue was a responsive committer that actually took the time a review the patches and came with constructive comments. Even just a comment that he/she will look into it later, made it feel good, and that my time wasn't wasted.

I think doing clean-ups doesn't help ju understand the JDT code, you just follow a pattern without the need to know what the code actually do. Sure you learn how to setup your IDE connect to GIT and commit changes to Gerrit, that was also a blocker for which I had to reach out for help to solve, so it might be a first entry for some.

Regards,

Kenneth

Den 2020-05-27 kl. 19:46, skrev [hidden email]:

Anything in the order of "attracting contributors"?

Is it more attractive to start working on a component by making clean up changes? Do clean up changes help to get involved and serve as motivation to start working on functional changes, too?

Have you observed difficulties in understanding JDT code, that were resolved by doing a clean up change first? Examples?

thanks,
Stephan

Am 2020-05-27 16:40, schrieb [hidden email]:

To really get the full picture, I would very much like to hear from our new contributors, how they see the connection between clean up changes and functional changes / bug fixes.

Is there any connection or are these disjoint activities?

If connected, how exactly?

thanks,
Stephan

Am 2020-05-27 16:21, schrieb [hidden email]:

Still one more suggestion / request:

Let's please discuss this JDT issue as a JDT issue only.

Platform is different. Hence also the p.o.v. of the Eclipse PMC is different from the day-to-day work in JDT.

Let's find out what's best for JDT.

thanks,
Stephan

Am 2020-05-27 13:10, schrieb [hidden email]:

In my post I mainly wanted to raise awareness that JDT code (even if x-internal) is potentially consumed outside JDT, and that even seemingly trivial changes can (and do) cause havoc downstream.

Now that the discussion has been broadened to the general issue of clean ups, I would like to list three kinds of clean-ups that I do consider useful:

1. Refactorings that help fixing a bug. This could be (a) a refactoring as part of the process of understanding some old code section, or (b) a refactoring that prepares for the desired solution.

2. Changes that improve the ability to detect potential bugs using JDT's own analysis, like avoiding raw types, adding null annotations (careful when API is affected!).

3. Refactorings that are performed for the purpose of testing our own functionality in a dog-fooding like approach.


I suggest that (1) and (2) are encouraged on our productive code base, and that branches are created for experiments in (3). These branches can be made available for voluntary field testing but should not be merged to master.

Types (1) and (2) need a bugzilla for every change.

If (3) is performed on a branch, perhaps one umbrella bug can cover several experiments.

best,
Stephan


Am 2020-05-26 20:55, schrieb Stephan Herrmann:

Hi,

Another episode in the question whether clean up changes are worth the effort they cause.

Today the Object Teams build got broken by https://git.eclipse.org/r/#/c/155226/ (which doesn't even have a bug that I could re-open).

Object Teams has tons of tests for checking that we don't break JDT. In that context we have a subclass of org.eclipse.jdt.testplugin.JavaProjectHelper. This no longer compiles since the above change.

Granted, the package is marked x-internal, so JDT has permission to change any way we want.

OTOH note that every project that extends JDT is potentially interested in using also code from the JDT test suite. Here we speak of a fairly large number of projects.

I would not complain if the change was necessary to implement new functionality or fix a bug, that's certainly covered by x-internal. But I strongly doubt that this "clean up" has a benefit that justifies the consequences.

What problem is solved by adding private constructors? Are you doing it just because it is possible? The commit message doesn't indicate you even thought of the possibility that s.o. would subclass those classes.

It's too late for changing the code, because I need to fix this today for M3.

But please keep this in mind when doing further clean-up.

Stephan
_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev



_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev



_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev



_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev



_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev

_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
Reply | Threaded
Open this post in threaded view
|

Re: "clean up" again

Gayan Perera
A clean structured code will help new comers to read it easily. I guess for JDT masters it might not make a difference since you have lived with the code and you might have written that code as well. Any code base deteriorate over time how much good you write it. So frequent cleanups and refactoring it essential. I this this problem have not been such a issue if object team had a proper ci pipeline building against latest JDT and these cleanups have been merged much earlier. 

Best regards,
Gayan

On Wed, 27 May 2020 at 20:54, Jeff Johnston <[hidden email]> wrote:
Not me personally, but I have been reviewing a number of cleanup changes since I started as a JDT committer.  Carsten Hammer has contributed a large number of these.
That said, I have noticed that Carsten has started to test and is opening bugs, creating test cases, and has recently started providing the odd patch so he is
gradually dipping his toes into contributing more than just cleanups.

-- Jeff J.

On Wed, May 27, 2020 at 2:25 PM Kenneth Styrberg <[hidden email]> wrote:

The main point for me was to fix bugs. At first when I did my first patches I also removed all warnings from the code, but was encouraged not to do so to simplify reviews.

To do clean-ups as a starting point wasn't a thing for me.  Maybe a more junior programmer finds that more gratifying than me. My main motivation to continue was a responsive committer that actually took the time a review the patches and came with constructive comments. Even just a comment that he/she will look into it later, made it feel good, and that my time wasn't wasted.

I think doing clean-ups doesn't help ju understand the JDT code, you just follow a pattern without the need to know what the code actually do. Sure you learn how to setup your IDE connect to GIT and commit changes to Gerrit, that was also a blocker for which I had to reach out for help to solve, so it might be a first entry for some.

Regards,

Kenneth

Den 2020-05-27 kl. 19:46, skrev [hidden email]:

Anything in the order of "attracting contributors"?

Is it more attractive to start working on a component by making clean up changes? Do clean up changes help to get involved and serve as motivation to start working on functional changes, too?

Have you observed difficulties in understanding JDT code, that were resolved by doing a clean up change first? Examples?

thanks,
Stephan

Am 2020-05-27 16:40, schrieb [hidden email]:

To really get the full picture, I would very much like to hear from our new contributors, how they see the connection between clean up changes and functional changes / bug fixes.

Is there any connection or are these disjoint activities?

If connected, how exactly?

thanks,
Stephan

Am 2020-05-27 16:21, schrieb [hidden email]:

Still one more suggestion / request:

Let's please discuss this JDT issue as a JDT issue only.

Platform is different. Hence also the p.o.v. of the Eclipse PMC is different from the day-to-day work in JDT.

Let's find out what's best for JDT.

thanks,
Stephan

Am 2020-05-27 13:10, schrieb [hidden email]:

In my post I mainly wanted to raise awareness that JDT code (even if x-internal) is potentially consumed outside JDT, and that even seemingly trivial changes can (and do) cause havoc downstream.

Now that the discussion has been broadened to the general issue of clean ups, I would like to list three kinds of clean-ups that I do consider useful:

1. Refactorings that help fixing a bug. This could be (a) a refactoring as part of the process of understanding some old code section, or (b) a refactoring that prepares for the desired solution.

2. Changes that improve the ability to detect potential bugs using JDT's own analysis, like avoiding raw types, adding null annotations (careful when API is affected!).

3. Refactorings that are performed for the purpose of testing our own functionality in a dog-fooding like approach.


I suggest that (1) and (2) are encouraged on our productive code base, and that branches are created for experiments in (3). These branches can be made available for voluntary field testing but should not be merged to master.

Types (1) and (2) need a bugzilla for every change.

If (3) is performed on a branch, perhaps one umbrella bug can cover several experiments.

best,
Stephan


Am 2020-05-26 20:55, schrieb Stephan Herrmann:

Hi,

Another episode in the question whether clean up changes are worth the effort they cause.

Today the Object Teams build got broken by https://git.eclipse.org/r/#/c/155226/ (which doesn't even have a bug that I could re-open).

Object Teams has tons of tests for checking that we don't break JDT. In that context we have a subclass of org.eclipse.jdt.testplugin.JavaProjectHelper. This no longer compiles since the above change.

Granted, the package is marked x-internal, so JDT has permission to change any way we want.

OTOH note that every project that extends JDT is potentially interested in using also code from the JDT test suite. Here we speak of a fairly large number of projects.

I would not complain if the change was necessary to implement new functionality or fix a bug, that's certainly covered by x-internal. But I strongly doubt that this "clean up" has a benefit that justifies the consequences.

What problem is solved by adding private constructors? Are you doing it just because it is possible? The commit message doesn't indicate you even thought of the possibility that s.o. would subclass those classes.

It's too late for changing the code, because I need to fix this today for M3.

But please keep this in mind when doing further clean-up.

Stephan
_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev



_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev



_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev



_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev



_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev

_______________________________________________
jdt-dev mailing list
[hidden email]
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jdt-dev
123