Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Infer expressions if there is no selection range when extracting method #1585

Merged
merged 1 commit into from
Nov 13, 2020
Merged

Infer expressions if there is no selection range when extracting method #1585

merged 1 commit into from
Nov 13, 2020

Conversation

CsCherrYY
Copy link
Contributor

@CsCherrYY CsCherrYY commented Oct 28, 2020 •
edited
Loading

Relates to redhat-developer/vscode-java#1680

Signed-off-by: Shi Chen chenshi@microsoft.com

@eclipse-ls-bot
Copy link

Can one of the admins verify this patch?

@CsCherrYY CsCherrYY mentioned this pull request Oct 28, 2020
Merged
@rgrunber
Copy link
Contributor

I haven't looked too thoroughly yet but if a client doesn't support the quick pick prompt, would this still work ? Is there a registered capability that can be checked to simply apply the "extract method" to the smallest applicable node when not supported ?

@testforstephen
Copy link
Contributor

Agree. It's necessary to add a switch such as inferExpressionForExtractRefactoringSupport to control whether to enable the capability. Besides, looks like infer a proper expression can also be applied to extract to local variable and field, would you consider to make them to reuse the same code?

@CsCherrYY
Copy link
Contributor Author

I haven't looked too thoroughly yet but if a client doesn't support the quick pick prompt, would this still work ? Is there a registered capability that can be checked to simply apply the "extract method" to the smallest applicable node when not supported ?

Agree. It's necessary to add a switch such as inferExpressionForExtractRefactoringSupport to control whether to enable the capability. Besides, looks like infer a proper expression can also be applied to extract to local variable and field, would you consider to make them to reuse the same code?

Thanks a lot for both of your suggestions.
@rgrunber, Since the current capability about extract method supports rename function, a new capability supporting to infer expression is needed, just as @testforstephen said.
I will explore and consider if extract variable and extract field can use the same (or similar) implementation as this. If yes, the new capability will support all the extract refactor process.

@fbricon fbricon added this to the Mid November 2020 milestone Nov 3, 2020
@rgrunber
Copy link
Contributor

I intend to review this by the end of the week.

@rgrunber
Copy link
Contributor

The change looks good to me. Feel free to merge. I'm guessing each change gets squashed into one prior to the merge ?

One thing I did notice, though not related to this change, is that the extract method generates a rename refactoring that sometimes fails to apply. I haven't looked too closely but it seems like it isn't tracking the extraction in some cases.

@CsCherrYY
Infer expressions if there is no selection range
58af608 
Signed-off-by: Shi Chen <chenshi@microsoft.com>
@CsCherrYY
Copy link
Contributor Author

The change looks good to me. Feel free to merge. I'm guessing each change gets squashed into one prior to the merge ?

One thing I did notice, though not related to this change, is that the extract method generates a rename refactoring that sometimes fails to apply. I haven't looked too closely but it seems like it isn't tracking the extraction in some cases.

I have squashed all the commits. As for the problem that renaming fails to apply, is there any related issue? Could you give some cases to reproduce?

@testforstephen
Copy link
Contributor

@rgrunber Will merge this PR first. Once you meet the problem again, feel free to create an issue and we're going to have a look again.

@testforstephen testforstephen merged commit 8308265 into eclipse-jdtls:master Nov 13, 2020
mfussenegger added a commit to mfussenegger/nvim-jdtls that referenced this pull request Nov 13, 2020
@mfussenegger
Add infer selection support for extractMethod
b3dcf7b 
Requires eclipse-jdtls/eclipse.jdt.ls#1585
@mfussenegger mfussenegger mentioned this pull request Nov 13, 2020
Merged
mfussenegger added a commit to mfussenegger/nvim-jdtls that referenced this pull request Nov 20, 2020
@mfussenegger
Add infer selection support for extractMethod
62d466b 
Requires eclipse-jdtls/eclipse.jdt.ls#1585
@CsCherrYY CsCherrYY deleted the extractMethodEnhance branch November 30, 2020 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@testforstephen testforstephen testforstephen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Mid November 2020
Development

Successfully merging this pull request may close these issues.
5 participants
@CsCherrYY @eclipse-ls-bot @rgrunber @testforstephen @fbricon