-
Notifications
You must be signed in to change notification settings - Fork 81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add quick assistance for field rename refactoring #751
Conversation
@jjohnstn We changed the code based on your suggestions, please kindly have a look on the resubmitted PR. |
Will look at it, thx. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @DongChunHao thanks for the patch. There a few comments you need to address. Mostly to do with the logic which needs to use bindings instead of name comparison which is unreliable.
....ui/ui/org/eclipse/jdt/internal/ui/text/correction/ConvertFieldNamingConventionProposal.java
Outdated
Show resolved
Hide resolved
....ui/ui/org/eclipse/jdt/internal/ui/text/correction/ConvertFieldNamingConventionProposal.java
Outdated
Show resolved
Hide resolved
....ui/ui/org/eclipse/jdt/internal/ui/text/correction/ConvertFieldNamingConventionProposal.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/correction/QuickAssistProcessor.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/correction/QuickAssistProcessor.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/correction/QuickAssistProcessor.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/correction/QuickAssistProcessor.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/correction/QuickAssistProcessor.java
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,145 @@ | |||
/******************************************************************************* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class should be in: org.eclipse.jdt.internal.ui.text.correction.proposals
....ui/ui/org/eclipse/jdt/internal/ui/text/correction/ConvertFieldNamingConventionProposal.java
Outdated
Show resolved
Hide resolved
0867125
to
ddf2595
Compare
@jjohnstn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @DongChunHao Thanks for the previous changes. A few more changes to finish this. See new comments. You also need to add tests to the AssistQuickFixTest class.
org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/correction/QuickAssistProcessor.java
Show resolved
Hide resolved
} | ||
|
||
private boolean isValidConstantName(String identifier) { | ||
Pattern pattern= Pattern.compile("^[A-Z0-9]+(_[A-Z0-9]+)*$"); //$NON-NLS-1$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make the pattern a final static variable in the class so it only needs to be evaluated once.
return false; | ||
} | ||
|
||
if (resultingCollections == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need a check that the new name isn't already used. At this point, you can use the FieldDeclaration to find it's AbstractTypeDeclaration parent and then do an ASTVisitor to find FieldDeclarations and mark whether there is already a field with newName. The refactoring is catching it, but you shouldn't offer the assist if this is the case.
|
||
@Override | ||
public String getAdditionalProposalInfo() { | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build an NLS string here that says something like Rename 'recLimit' to 'REC_LIMIT' so something shows up in the 2nd window like you see when you click on Create getter and setter for x... You can use positional variables in the NLS string.
e.g. ConvertFieldUsingConstantNaming_additional_info=Rename the field ''{0}'' to ''{1}''
and then use the Messages.format() to add the parameters to the string that you will return.
@jjohnstn |
/rebase |
75798f2
to
f097329
Compare
/rebase |
f097329
to
085decb
Compare
/rebase |
085decb
to
5e8816b
Compare
@DongChunHao I made some additional fixes. For the future, you cannot hard-code strings that get displayed as they need to be translatable so must be put into properties files. The comment I made about the Pattern was to do the compile as part of a final static variable so the compile only occurs once for the class. Anyway, done. |
Hi, @jjohnstn Thank you very much for your assistance, teaching me how to modify the code and pay attention to development standards. I will be more mindful in the future. Thanks again, and all the best. |
This causes regression, see #820 |
What it does
How to test
Author checklist