Automated Name Recommendation For The Extract Local Variable Refactoring #602

Michaelll123 · 2023-05-29T13:53:55Z

modify StubUtility() to add new rules to generate names for extracted local variables.
add popular plural words (irregular cases) to ConvertLoopOperation.java to support the singularization of plural words.
add test to ExtractTempTests.
fixes Automated Name Recommendation For The Extract Local Variable Refactoring #601 .

What it does

see issue cited.

How to test

Run test153, and test154 in ExtractTempTests.

Author checklist

I have thoroughly tested my changes
The change is following the coding conventions
I have signed the Eclipse Contributor Agreement (ECA)

iloveeclipse · 2023-05-29T16:40:18Z

...jdt.core.manipulation/common/org/eclipse/jdt/internal/core/manipulation/EnglishWordUtil.java

+package org.eclipse.jdt.internal.core.manipulation;
+/**
+ * This utility class for English words is taken from a blog on CSDN, and we modify it to adapt for the cases of multiply word.
+ * The original blog can be found in https://blog.csdn.net/weixin_44258855/article/details/123201283


Sorry, this can't be merged into eclipse.org codebase as it refers to a source without clear licensing information, doesnt represent your own code and the snippet itself seem to be largely inadequate to address the problem you are trying to fix.

Thanks, I will try to solve this problem soon.

@iloveeclipse how did you determine the source of code ? i.e if its devs own or not ? Is there some tool for it ?

Are third-party APIs allowed to merge into eclipse.org codebase?

See https://www.eclipse.org/projects/handbook/#ip

TL;DR: content added to the jdt.ui bundle must be licensed under EPL license.

@iloveeclipse how did you determine the source of code ? i.e if its devs own or not ? Is there some tool for it ?

I would name it BDA "brain driven analysis". Of course as every tool that might fail too.
In this concrete context I just assume the "using code posted by someone on some blog page" is most likely description of the code not written by PR author and so it is highly questionable if that is allowed to be merged here.

It looks like there is no existing open-source tool written in Java to support the convertion between plural words and singular words. I will implement my own version soon.

@Michaelll123 FWIW: a lot of English words are unnecessary to handle (e.g. geese to goose is unlikely to show up in code and not worth the time to look for it). There was a pared down version of finding a singular in ConvertLoopOperation.modifyBaseName() which gets a variable name for an enhanced-for-loop (e.g. for (String name : names)). It handled a number of common usages (e.g. children vs child) and then identified odd English plural suffixes to avoid attempting removing the "s" in which case it defaulted to "element". You could base off of that and add more direct conversion cases if you want. Pure plural support is over-kill IMO.

@jjohnstn Thanks for the insightful advice! It is indeed a tricky problem to cover all the conversion cases in English, and I will start with ConvertLoopOperation.modifyBaseName().

Thanks for all your valuable advice, and I have finished the requested changes. Please kindly review the latest pull request.

jjohnstn

Overall, the logic is fine and additions to the loop name logic are useful. The logic can be moved into another method in StubUtility and cleaned up a bit.

...re.manipulation/core extension/org/eclipse/jdt/internal/corext/fix/ConvertLoopOperation.java

...pse.jdt.core.manipulation/common/org/eclipse/jdt/internal/core/manipulation/StubUtility.java

jjohnstn · 2023-06-02T23:59:40Z

...pse.jdt.core.manipulation/common/org/eclipse/jdt/internal/core/manipulation/StubUtility.java

+					MethodInvocation methodInvocation= (MethodInvocation)assignedExpression;
+					Expression receiver= methodInvocation.getExpression();
+					String methodName= methodInvocation.getName().toString();
+					if(receiver != null) {


You should be checking for next() being called before bothering with receiver. I also think this logic could be put into getBaseNameFromExpression() as it is only used by the getVariableNameSuggestions() methods.

Thanks for the review, and I have put my code snippets into getBaseNameFromExpression.

Looks good.

Michaelll123 · 2023-06-03T08:40:53Z

Thanks for all your valuable advice, and I have finished the requested changes. Please kindly review the latest pull request.

jjohnstn

Looks good. Just a formatting issue with casting adding a blank. Fix that and it is ready to go.

jjohnstn · 2023-06-06T03:04:43Z

...pse.jdt.core.manipulation/common/org/eclipse/jdt/internal/core/manipulation/StubUtility.java

 		} else if (assignedExpression instanceof SuperMethodInvocation) {
-			name= ((SuperMethodInvocation)assignedExpression).getName().getIdentifier();
+			name= ((SuperMethodInvocation) assignedExpression).getName().getIdentifier();


Something in your format profile is adding a blank between casts and their element to cast. This should not be changed as part of this patch. Other than that, the code looks good. If you want me to fix it with my own formatting, I can do so.

Thanks a lot! I will fix that immediately.

Michaelll123 · 2023-06-06T07:26:28Z

The format issues have been addressed. Looking forward to being merged.

Code that was in question has been replaced by existing code in for loop cleanup.

jjohnstn

Hi @Michaelll123 The patch is fine but please recreate the patch in your branch so it is rebased on master and submit via --force push. You have been merging master into your branch which is disallowed. You should always rebase your patches on top of master as opposed to merging master into your branch.

Michaelll123 · 2023-06-07T08:18:46Z

Thanks for the review. I think the latest patch is based on master, and there are no commits merging master into recommend-name-for-extract-local-variables. Please check again, thanks!

jjohnstn · 2023-06-07T20:46:18Z

Hi @Michaelll123 Looks good. Just need to wait until master opens for 4.29 M1

Michaelll123 · 2023-06-08T00:34:06Z

Thanks! Have a good one!

iloveeclipse previously requested changes May 29, 2023

View reviewed changes

Michaelll123 requested a review from iloveeclipse June 1, 2023 03:05

jjohnstn requested changes Jun 3, 2023

View reviewed changes

Michaelll123 requested a review from jjohnstn June 3, 2023 08:38

jjohnstn requested changes Jun 6, 2023

View reviewed changes

Michaelll123 requested a review from jjohnstn June 6, 2023 07:23

jjohnstn added this to the 4.29 M1 milestone Jun 6, 2023

jjohnstn added the enhancement New feature or request label Jun 6, 2023

jjohnstn self-assigned this Jun 6, 2023

jjohnstn approved these changes Jun 6, 2023

View reviewed changes

jjohnstn requested changes Jun 6, 2023

View reviewed changes

Michaelll123 force-pushed the recommend-name-for-extract-local-variables branch 3 times, most recently from de74dbe to 03951e9 Compare June 7, 2023 01:24

Michaelll123 closed this Jun 7, 2023

Michaelll123 force-pushed the recommend-name-for-extract-local-variables branch from 03951e9 to c5f4aa6 Compare June 7, 2023 01:44

Michaelll123 reopened this Jun 7, 2023

resubmit

5e741f5

Michaelll123 force-pushed the recommend-name-for-extract-local-variables branch from 54df185 to 5e741f5 Compare June 7, 2023 07:49

Michaelll123 requested a review from jjohnstn June 7, 2023 08:19

jjohnstn approved these changes Jun 7, 2023

View reviewed changes

jjohnstn added 3 commits June 8, 2023 15:34

Version bumps

4ad30ce

More version bumps

1d6612d

And more version bumps

586afa1

jjohnstn merged commit bb51805 into eclipse-jdt:master Jun 8, 2023

Michaelll123 mentioned this pull request Apr 7, 2024

Improving the Names Recommended by Extract Local Variable Refactoring eclipse-cdt/cdt#754

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automated Name Recommendation For The Extract Local Variable Refactoring #602

Automated Name Recommendation For The Extract Local Variable Refactoring #602

Michaelll123 commented May 29, 2023 •

edited

Loading

iloveeclipse May 29, 2023

Michaelll123 May 30, 2023

aino-gautam May 31, 2023

iloveeclipse May 31, 2023

iloveeclipse May 31, 2023

Michaelll123 May 31, 2023

jjohnstn May 31, 2023

Michaelll123 Jun 1, 2023

Michaelll123 Jun 2, 2023

jjohnstn left a comment

jjohnstn Jun 2, 2023

Michaelll123 Jun 3, 2023

jjohnstn Jun 6, 2023

Michaelll123 commented Jun 3, 2023

jjohnstn left a comment

jjohnstn Jun 6, 2023

Michaelll123 Jun 6, 2023

Michaelll123 commented Jun 6, 2023

jjohnstn left a comment

Michaelll123 commented Jun 7, 2023

jjohnstn commented Jun 7, 2023

Michaelll123 commented Jun 8, 2023

Automated Name Recommendation For The Extract Local Variable Refactoring #602

Automated Name Recommendation For The Extract Local Variable Refactoring #602

Conversation

Michaelll123 commented May 29, 2023 • edited Loading

What it does

How to test

Author checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjohnstn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michaelll123 commented Jun 3, 2023

jjohnstn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michaelll123 commented Jun 6, 2023

jjohnstn left a comment

Choose a reason for hiding this comment

Michaelll123 commented Jun 7, 2023

jjohnstn commented Jun 7, 2023

Michaelll123 commented Jun 8, 2023

Michaelll123 commented May 29, 2023 •

edited

Loading