search

JnRouvignac / AutoRefactor

Eclipse plugin to automatically refactor Java code bases
Other
174 stars 37 forks source link

Contribute to JDT cleanup actions #386

Open vogella opened 5 years ago

vogella commented 5 years ago

Hello, I just found your plug-in and I'm already in love with it. Thanks for this awesome work.

Would you be interested in moving parts of your plug-in to JDT cleanup actions? I think this would be highly beneficial for all Eclipse users.

P.S. I'm one of the Eclipse developers and project leads. See https://projects.eclipse.org/projects/eclipse.platform/who

vogella commented 5 years ago

Btw. I'm starting to use your tool now to improve the Eclipse code. See for example: https://git.eclipse.org/r/c/144024/ and https://git.eclipse.org/r/c/144029/

:-)

strkkk commented 5 years ago

@vogella sounds interesting. What refactorings are most useful? Is there some guide how to add new JDT cleanup actions?

Btw, thank you for your site and tutorials, they were very useful for me.

Fabrice-TIERCELIN commented 5 years ago

It's the best future we hope for this plugin! We don't know if we will be here in a decade. This sounds like immortality :)

The current development version has transformed the plugin as a CleanUp Extension. It should ease the migration. There is still one limitation: in this mode, AutoRefactor only does 1 pass. I think I know how to fix it but it's not already done.

vogella commented 5 years ago

Awesome. One of our employees recently added a cleanup action to JDT so we should be able to help here. :-)

Maybe let's start with "Combine try catch blocks"?

Fabrice TIERCELIN notifications@github.com schrieb am Fr., 14. Juni 2019, 20:13:

It's the best future we hope for this plugin! We don't know if we will be here in a decade. This sounds like immortality :)

The current development version has transformed the plugin as a CleanUp Extension. It should ease the migration. There is still one limitation: in this mode, AutoRefactor only does 1 pass. I think I know how to fix it but it's not already done.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JnRouvignac/AutoRefactor/issues/386?email_source=notifications&email_token=AABCFBTOZPU6V37U54XX2TTP2PNS3A5CNFSM4HYFPID2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXXSERI#issuecomment-502211141, or mute the thread https://github.com/notifications/unsubscribe-auth/AABCFBS2ASGU36IICJHO4JTP2PNS3ANCNFSM4HYFPIDQ .

Fabrice-TIERCELIN commented 5 years ago

Do you mean UseMultiCatchRefactoring.java rule? Push negation down rule seems to be easier to integrate. The code is more simple, there is less dependency and it's useful.

I think we should check the policy:

  1. Some rules erase some comments (not Push negation down). Perhaps, we should had a check and abort the refactoring if a comment will be lost.
  2. Some rules need little cleaning from the other rules (Push negation down for instance) like removing useless parenthesis. The user doesn't see it as all the rules are running together but it can be a problem if we pick up one rule. Perhaps we need to rework the rules to make them self sufficient.
  3. Some rules may trigger what I call zombie code (not Push negation down). Zombie code is a code that is dead because of an error above, but it can be triggered if the bug is fixed. For example, the Equals on constant rather than on variable rule avoids some null pointers, which should be a good thing, but in some case the null pointer is caught and it disables a zombie code. I think we should process the concerned rules later.
vogella commented 5 years ago

Remove useless blocks would also be a very nice addition, opened https://bugs.eclipse.org/bugs/show_bug.cgi?id=548324 for that.

vogella commented 5 years ago

Testing now Push negation down.

vogella commented 5 years ago

By the way, should we start direct communication? If you agree, please drop me an email at Lars.Vogel@vogella.com

vogella commented 5 years ago

"Push negation down" does not have one hit in the eclipse.platform code base so I think this is not the most valuable clean for Eclipse users.

I had lots of:

1.) contains vrs. indexOf cases 2.) Remove useless block 3.) Use try-with-resource 4.) Use String.contains 5.) Use multi-catch 6.) Remove empty ifs 7.) Remove unnecessary local variables before return statement 8.) All in one method, rather than loop

Do you have a convert to method reference refactoring? That would also be beneficial.

Fabrice-TIERCELIN commented 5 years ago

LambdaRefactoring.java converts a lambda expression into a method reference if possible.

vogella commented 5 years ago

What is the next step?

Would you like start contributing cleanup actions / quickfixes to JDT? For this we use Gerrit, if you have issues with this process, I can help with that. JDT is not very active in terms of reviewing community contributions but I could ask the Redhat team to review. If you start this, I think it would be great if you would gain JDT committership at some point in time to improve these cleanups directly.

Fabrice-TIERCELIN commented 5 years ago

You said "One of our employees recently added a cleanup action to JDT". Can I see the diff?

Another question: is cleanup action multi-pass? I mean:

Given

boolean reducedValue = !!!!true;

When

Cleanup is applied

Then

A multi-pass process can reevaluate an already changed part of code. It's coded in AutoRefactor. If it is not the case in cleanup, I think it would be a good idea to implement it.

jjohnstn commented 5 years ago

Hi Fabrice, I have recently been made a JDT committer. I can help if you have code to be reviewed or have questions though I am relatively new and am still learning myself.

vogella commented 5 years ago

Jeff, can you give the link to your ; cleanup Gerrit?

jjohnstn notifications@github.com schrieb am Di., 25. Juni 2019, 18:37:

Hi Fabrice, I have recently been made a JDT committer. I can help if you have code to be reviewed or have questions though I am relatively new and am still learning myself.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JnRouvignac/AutoRefactor/issues/386?email_source=notifications&email_token=AABCFBR4DWNZ63UFFHSIYS3P4JCURA5CNFSM4HYFPID2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYQ27LY#issuecomment-505524143, or mute the thread https://github.com/notifications/unsubscribe-auth/AABCFBQH6OQYSRPJMAPCQSTP4JCURANCNFSM4HYFPIDQ .

jjohnstn commented 5 years ago

With regards to single-pass vs multi-pass, the cleanups are not recursively checked, however, cleanups should be smart (e.g. I wrote a redundant semi-colon cleanup that will remove multiple extraneous trailing semi-colons at once and not just one at a time). Your example above could be calculated to a single change. An example where this doesn't work is removing unused private members/fields where a private field is being used by an unused private method. First cleanup removes the unused method, second cleanup will remove the unused field.

My gerrit change for redundant semi-colons can be found at:

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

Fabrice-TIERCELIN commented 5 years ago

@vogella, here we are, I have made a first contribution. I searched the simplest useful rule. I have complete the cleanup that removes the unnecessary modifiers. Now it will also remove the abstract modifier on interface : Remove_abstract_modifier_on_interface.patch

Now, I don't know how I'm supposed to contribute. Can I create an entry in Eclipse bugzilla or you create it?

Fabrice-TIERCELIN commented 5 years ago

I have been able to test the feature on Eclipse but I haven't been able to run the unitary tests. Maybe you can do it.

vogella commented 5 years ago

@Fabrice-TIERCELIN awesome. I'm already in summer vacation, so it would be great if you can open a bug directly. Please use "UI" and the following link: https://bugs.eclipse.org/bugs/enter_bug.cgi?product=JDT

This requires a bugzilla user.

We use Gerrit for code contributions, as for the setup, please see https://www.vogella.com/tutorials/EclipsePlatformDevelopment/article.html#exericse-eclipse-user-creation-and-gerrit-server-configuration

The setup usually only takes 10 minutes in our hackerthons, we do from time to time in Hamburg.

For the Gerrit, please add Jeff (Jeff Johnston from Redhat) as reviewer. Review times in JDT can be very long, adding Jeff (or Roland Grunberg if Jeff is not available) as reviewer should result in faster review.

I'm happy to help with any Gerrit questions, important rule is to use the same Change-Id if you want to update the review. Eclipse Staging View has nice Gerrit support and the ammend option can be used to update Gerrits.

vogella commented 5 years ago

Gerrit will run the test automatically, no need for you to do it

Fabrice TIERCELIN notifications@github.com schrieb am Mo., 8. Juli 2019, 08:19:

I have been able to test the feature on Eclipse but I haven't been able to run the unitary tests. Maybe you can do it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JnRouvignac/AutoRefactor/issues/386?email_source=notifications&email_token=AABCFBWOO2Q367HO2RD7QYLP6LL7RA5CNFSM4HYFPID2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZMCR5I#issuecomment-509094133, or mute the thread https://github.com/notifications/unsubscribe-auth/AABCFBUHIXUECVUENXVWO4LP6LL7RANCNFSM4HYFPIDQ .

vogella commented 5 years ago

Awesome @Fabrice-TIERCELIN, thanks for https://bugs.eclipse.org/bugs/show_bug.cgi?id=549242. Any plans to contribute more cleanups?

Fabrice-TIERCELIN commented 5 years ago

I'm preparing the migration of Autoboxing and Unboxing.

akurtakov commented 5 years ago

@Fabrice-TIERCELIN thanks for doing it - would you please watch https://twitter.com/ZhekaKozlov/status/1135506701438857217 . I'm interested whether there are other of your cleanup actions that can help us achieve parity (boxing/unboxing should be one of them).

Fabrice-TIERCELIN commented 5 years ago

@akurtakov What is this tool? What is the list of the features?

PyvesB commented 5 years ago

Awesome work, looking forward to seeing more features getting migrated! :+1:

Fabrice-TIERCELIN commented 5 years ago

Hi @vogella, do you know how your Jenkins is configured to feature the build SUCCESS as green? On my Jenkins, it appears in blue...

Or do you know who knows?

PyvesB commented 5 years ago

Hi @vogella, do you know how your Jenkins is configured to feature the build SUCCESS as green? On my Jenkins, it appears in blue...

Or do you know who knows?

You'll have to install the Green Balls plugin. :wink:

Fabrice-TIERCELIN commented 5 years ago

OK, thanks @PyvesB.

Fabrice-TIERCELIN commented 5 years ago

Awesome work, looking forward to seeing more features getting migrated! 👍

@PyvesB, @akurtakov and @vogella, don't support the coder, support the reviewer. The migration time is estimated to 6 years and it's mostly review time.

Or be a merger 😺

vogella commented 5 years ago

Eclipse ist currently in release freeze but we should open master soon again. And I assume this means reviews will continue.

Fabrice TIERCELIN notifications@github.com schrieb am Sa., 7. Sep. 2019, 15:58:

Awesome work, looking forward to seeing more features getting migrated! 👍

@PyvesB https://github.com/PyvesB, @akurtakov https://github.com/akurtakov and @vogella https://github.com/vogella, don't support the coder, support the reviewer. The migration time is estimated to 6 years and it's mostly review time https://bugs.eclipse.org/bugs/show_bug.cgi?id=550394.

Or be a merger 😺

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JnRouvignac/AutoRefactor/issues/386?email_source=notifications&email_token=AABCFBRGIATQE6OJW3IZKVDQIOXSDA5CNFSM4HYFPID2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6EZNFI#issuecomment-529110677, or mute the thread https://github.com/notifications/unsubscribe-auth/AABCFBUZPIUW2IEDF5LKNR3QIOXSDANCNFSM4HYFPIDQ .

Fabrice-TIERCELIN commented 5 years ago

OK, what do you prefer, what will be faster:

vogella commented 5 years ago

Several rules in several tickets is definitely a good strategy.

I would love to see you becoming JDT committer at some point so that you can work directly on the code. This typically requires multiple contributions.

On Mon, Sep 9, 2019 at 7:50 AM Fabrice TIERCELIN notifications@github.com wrote:

OK, what do you prefer, what will be faster:

  • One rule at a time?
  • Several rules in several tickets?
  • Several rules in one ticket?
  • Another solution...?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JnRouvignac/AutoRefactor/issues/386?email_source=notifications&email_token=AABCFBWXHPT2IK6E7PPNSG3QIXP3RA5CNFSM4HYFPID2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6GKBKQ#issuecomment-529309866, or mute the thread https://github.com/notifications/unsubscribe-auth/AABCFBSABIPPHB3XTVQ4CMLQIXP3RANCNFSM4HYFPIDQ .

-- 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@vogella.com, Web: http://www.vogella.com

Fabrice-TIERCELIN commented 5 years ago

@vogella, can we find more mergers for JDT UI? There are 110 rules to merge. There is enough work for everybody :)

akurtakov commented 5 years ago

@Fabrice-TIERCELIN do you have many patches just sitting there?

Fabrice-TIERCELIN commented 5 years ago

Currently, I have submitted this and that but I can submit more rules at the same time if you want :D

akurtakov commented 5 years ago

I'm not JDT committer but I can influence two of the committers if review speed is too slow, that's why I asked. Unfortunately more than that we can achieve once you're committer too.

vogella commented 5 years ago

@Fabrice-TIERCELIN JDT Thanks a lot for continuing to work on this. JDT is still very low on active reviewers, I'm also not a committer. With Jeff and Roland from Redhat which become JDT committer not too long ago, we have at least two people which help with review and we hope to increase that number in the future. If is definitely fine to add them to reviews and remind them for feedback in the Gerrit from time to time.

I recommend submitting multiple rules at the same time. As soon as you have sufficient contributions, you can ask for commit rights and work on your own speed.

Off-topic: Do you have a refactoring to remove unnecessary final declarations? Would be a very useful addition for me to continue to cleanup the Eclipse code.

vogella commented 5 years ago

@Fabrice-TIERCELIN I see that some of your reviews have open feedback, like https://git.eclipse.org/r/c/148248/ please update them to the review comments, it might be that Jeff waits on your reaction before reviewing more of your contributions.

Fabrice-TIERCELIN commented 5 years ago

It is under control :) It is waiting for other patches to be merged.

To explain, this patch is too big and some other patches will progressively merge some parts of the code.

vogella commented 5 years ago

Always good If you could add these comments to the Gerrit review so that the reviewers understands your intention.

Fabrice TIERCELIN notifications@github.com schrieb am Di., 22. Okt. 2019, 19:33:

It is under control :) It is waiting for other patches to be merged.

To explain, this patch is too big and some other patches will progressively merge some parts of the code.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JnRouvignac/AutoRefactor/issues/386?email_source=notifications&email_token=AABCFBQFAMMZZRBE5FTTYHLQP42QLA5CNFSM4HYFPID2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEB6SDHA#issuecomment-545071516, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABCFBWX7GWTH3R5D2ZF3ILQP42QLANCNFSM4HYFPIDQ .

Fabrice-TIERCELIN commented 5 years ago

Unnecessary final declarations? The problem is that, except variables used in nested class, the final modifier is always unnecessary. It's only a help for the coder. So almost all the final modifier can be removed. In which cases the final modifier should be useless?

The only case the final modifier is useless is in interface method signature. You can already remove this with AutoRefactor and the Eclipse CleanUp.

vogella commented 5 years ago

It would be nice to have a cleanup to remove "Unnecessary final declarations". I think with a Java version (I think it was Java 8), the compiler is able to find out implicit final variables.

So almost all the final modifier can be removed.

Yes, would be great to have a cleanup for this.

Bye the way, I asked you a while if you could try your cleanups on Platform code and provide patches for them. I don't think you answered. If that something you could do? Would help to test the cleanups and in platform we have more reviewers.

Fabrice-TIERCELIN commented 5 years ago

Do you want to remove also the final modifier on method parameters?

vogella commented 5 years ago

+1

Fabrice TIERCELIN notifications@github.com schrieb am Mi., 23. Okt. 2019, 07:27:

Do you want to remove also the final modifier on method parameters?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JnRouvignac/AutoRefactor/issues/386?email_source=notifications&email_token=AABCFBVGPJMU5OTYJNOIKHTQP7OCLA5CNFSM4HYFPID2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECADIJQ#issuecomment-545272870, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABCFBS76JTOEE3E6HW2JJ3QP7OCLANCNFSM4HYFPIDQ .

Fabrice-TIERCELIN commented 5 years ago

How can I ask commit rights?

jjohnstn commented 5 years ago

Hi Fabrice, it is not just a simple case of asking for commit rights. What is required is a nomination to the JDT group. Before that, a body of work consisting of bug fixes / features needs to be established that prove you require minimal oversight. At this point, you are definitely on the right track to becoming nominated but there aren't enough merged bug fix changes yet (excludes code cleanups) and you also need to get a chance to demonstrate administrative aspects for bugs (milestones, status). I am confident you will get there in a shorter time than it took me.

Fabrice-TIERCELIN commented 5 years ago

I discovered a bug yesterday and I saw that my notifications on bugzilla were disabled in my preferences. Is there any other tickets I'm in CC?

vogella commented 5 years ago

Is there any other tickets I'm in CC?

I asked you a while ago if you could try your cleanups on Platform code and provide patches for them. But I also asked you already here and you also did not react so I assume this is your way of saying no.

Fabrice-TIERCELIN commented 4 years ago

I admit I'm not very OK to refactor a huge code I don't know... You also asked me to create a rule to remove the final keyword. As there are lots of activities around AutoRefactor, I prefer making rules for anyone than this rule...

I think you're right and that's probably the reason I haven't answered :) Anyway, which is the ticket for cleaning the Platform code?

vogella commented 4 years ago

I admit I'm not very OK to refactor a huge code I don't know. Understandable and thanks again for converting our cleanups to platform. Looking forward to see more of them, we starting to run the cleanups on the platform code.

Fabrice-TIERCELIN commented 4 years ago

Yet another cleanup: Use var keyword

It's the first cleanup I have coded that does not come from AutoRefactor. Meanwhile, here is my cleanups that can already been reviewed:

And also: