-
Notifications
You must be signed in to change notification settings - Fork 399
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
Support change signature refactoring #2497
Conversation
d23c0ec
to
263ff3c
Compare
test this please |
2722bee
to
0463b75
Compare
...clipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/ChangeSignatureHandler.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/TextEditConverter.java
Outdated
Show resolved
Hide resolved
test this please |
newExceptionInfos.add(ExceptionInfo.createInfoForAddedException(type)); | ||
} | ||
} else { | ||
IType type = cu.getJavaProject().findType(exception.type); |
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.
Can we do some context-aware search first here? For example:
- Check if the exception type appears in the import declarations.
- If it's
Exception
, just simply treat it asjava.lang.Exception
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.
Signed-off-by: Shi Chen <[email protected]>
Signed-off-by: Shi Chen <[email protected]>
Signed-off-by: Shi Chen <[email protected]>
Signed-off-by: Shi Chen <[email protected]>
2. find possible match types in existing import declarations 3. filter non-public types for Exception Signed-off-by: Shi Chen <[email protected]>
eda6e38
to
dffe5f4
Compare
...clipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/ChangeSignatureHandler.java
Show resolved
Hide resolved
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.
Overall looks good to me. We can address the issue separately. If more users are asking for it.
Would it make sense to add this behind a new extended client capability? In clients other than vscode this now shows a |
@mfussenegger Yes it makes sense. Would you like to raise a PR for that? |
eclipse-jdt/eclipse.jdt.ui#428 was merged, so feel free to create a PR with TP update, and with some reference changes we can claw back ~6500 (I think) of copied code 🚀 |
kudos! |
related to redhat-developer/vscode-java#2967, more details and demo videos can be found in that PR.
This PR includes several added files which can be excluded after eclipse-jdt/eclipse.jdt.ui#428 got merged. To be specific, the classes in
org.eclipse.jdt.internal
package.test cases can be found in
ChangeSignatureHandlerTest