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.

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 support to choose better name suggestion in extract constant #2414

Open
gayanper opened this issue Jan 22, 2023 · 8 comments
Open

Add support to choose better name suggestion in extract constant #2414

gayanper opened this issue Jan 22, 2023 · 8 comments

Comments

@gayanper
Copy link
Contributor

Take the following code snippet

public class Marvel {
  public static void main(String[] args) {
    updateName("Tony");
  }

  public static void updateName(String name) {
  }
}

Now if i try to extract a constant for the string literal, I only get the suggestion "TONY", even thought JDT produce more suggestions based on expected type and receiver, in this case the name of the parameter, There is no way to present them in vscode workflow. In this scenario I prefer to have "NAME" rather than "TONY".

Suggestion 1

Let use configure the order of the constant name computation in a settings like,

"java.refactor.extractVariable.newNameComputationOrder": ["receiverName", "literal", "expectedType"]

(The setting name is utterly ugly, but you get the idea :) )

Suggestion 2

This is bit invasive to the workflow, When we trigger extract constant, instead of rename, We will run a custom command to choose a constant name out of what was computed by JDT. That will be presented by the vscode in a selection list like the lists we use in "Generate Getters". Then the chosen value will be used to invoke the rename command, Or may be we might not need the rename command at all.

Since basic workflow needs to be supported without this extra step in other editors, this could be a client feature which server checks before deciding the additional command, whether it needs to be a rename or proposal selection. Also for vscode this can be controlled by a setting as well.

@gayanper
Copy link
Contributor Author

@rgrunber @jdneo @testforstephen @mickaelistria @snjeza WDYT ? Let me know your ideas as well. If you have a more simpler suggestion let me know as well. I would like to help to implement this feature.

@gayanper gayanper changed the title Add support to choose better extract constant suggestion Add support to choose better name suggestion in extract constant Jan 22, 2023
@snjeza
Copy link
Contributor

snjeza commented Jan 22, 2023

Suggestion 1

+1

@mickaelistria
Copy link
Contributor

Is this specifically about JDT-LS? Can't it be something to configure in JDT-Core?

@snjeza
Copy link
Contributor

snjeza commented Jan 22, 2023

@mickaelistria Eclipse JDT.UI also suggests TONY.

@gayanper
Copy link
Contributor Author

Is this specifically about JDT-LS? Can't it be something to configure in JDT-Core?

JDT UI works as expected suggesting TONY, NAME and STRING. But since jdt.ls can only offer one suggestion the implementation returns the first suggestion only. This is a limitation in LSP if I’m correct. The Suggestion 2 is the get around it and have similar functionality to IDEs offer.

@mickaelistria
Copy link
Contributor

But since jdt.ls can only offer one suggestion the implementation returns the first suggestion only. This is a limitation in LSP if I’m correct.

OK, that would be microsoft/language-server-protocol#592 I believe. It would be good to consider contributing this enhancement to LSP and LSP4J so JDT-LS could just leverage it like JDT. This would be a generally very useful feature; if JDT can drive to better LSP. that's a win for many people.

@rgrunber
Copy link
Contributor

rgrunber commented Jan 23, 2023

What @gayanper says, sound about right. I remember looking at this and realizing that the LSP doesn't have a way to support "linked editing model" the same way JDT-UI does. (eg. public void someMethod () where public can cycle through public, protected, private), so if some text edit exists as a "linked" edit, we take the first suggestion. That is the real problem.

The issue of smarter names has come up in the past, as redhat-developer/vscode-java#2011 & https://bugs.eclipse.org/bugs/show_bug.cgi?id=570785 .

Update: From quick look, the problem seems to be that the LSP spec doesn't support snippets for a TextEdit like it does for InsertTextFormat of a completion item. If that were supported, it could be easily done, and VS Code already has support for it through https://code.visualstudio.com/docs/editor/userdefinedsnippets#_choice

@mickaelistria
Copy link
Contributor

@rgrunber see microsoft/language-server-protocol#724 , microsoft/language-server-protocol#592 . Those are I believe the 2 most active issues of LSP with several dozens of incoming issues from LS implementations, including one from JDT-LS ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants