GRECLIPSE
  1. GRECLIPSE
  2. GRECLIPSE-1357

Add "Assign statement to new local variable" (CTRL+2L) quick fix

    Details

    • Type: Improvement Improvement
    • Status: Resolved Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.7.0.Release
    • Component/s: Quick fix/assist
    • Labels:
      None
    • Number of attachments :
      2

      Description

      This is a very commonly used quick fix in the Java editor. It allows you to do the following:

      new Object()//CTRL + 2L
      

      to produce:

      Object object = new Object()
      
      1. org.codehaus.groovy.eclipse.ui.patch
        4 kB
        Stephanie Van Dyk
      2. org.codehaus.groovy.eclipse.ui.patch
        39 kB
        Stephanie Van Dyk

        Activity

        Hide
        Stephanie Van Dyk added a comment -

        I've started working on this.

        Show
        Stephanie Van Dyk added a comment - I've started working on this.
        Hide
        Andrew Eisenberg added a comment -

        Have a look at org.eclipse.jdt.core.NamingConventions.suggestVariableNames(int, int, String, IJavaProject, int, String[], boolean).

        Not all of the fields will be relevant, and as we mentioned, there is no need to infer the type of the variable.

        One thing that we didn't mention is that it is not a good idea to propose any names that are already in scope. However, as a first pass at this, let's not worry about it.

        Lastly, we need to figure out how to hook this into the existing keybindings, but first get the basic working and then we will work on keybindings.

        Show
        Andrew Eisenberg added a comment - Have a look at org.eclipse.jdt.core.NamingConventions.suggestVariableNames(int, int, String, IJavaProject, int, String[], boolean) . Not all of the fields will be relevant, and as we mentioned, there is no need to infer the type of the variable. One thing that we didn't mention is that it is not a good idea to propose any names that are already in scope. However, as a first pass at this, let's not worry about it. Lastly, we need to figure out how to hook this into the existing keybindings, but first get the basic working and then we will work on keybindings.
        Hide
        Bob Tiernay added a comment -

        Something I just thought about was multiple assignment. Not sure if this is easy to support or should be supported, but it is something to think about.

        For example:

        [1,2]//CTRL + 2L
        

        by way of the refactoring could produce:

        def (x, y) = [1,2]
        

        Similarly:

        List f() {
          [1, 2]
        }
        ...
        f()//CTRL + 2L
        

        by way of the refactoring could produce:

        List f() {
          [1, 2]
        }
        ...
        def (x, y) = f()
        
        Show
        Bob Tiernay added a comment - Something I just thought about was multiple assignment. Not sure if this is easy to support or should be supported, but it is something to think about. For example: [1,2] //CTRL + 2L by way of the refactoring could produce: def (x, y) = [1,2] Similarly: List f() { [1, 2] } ... f() //CTRL + 2L by way of the refactoring could produce: List f() { [1, 2] } ... def (x, y) = f()
        Hide
        Bob Tiernay added a comment -

        Also, should the refactoring ask the user if the local variable should be typed? Or should it always use the inferred type?

        Show
        Bob Tiernay added a comment - Also, should the refactoring ask the user if the local variable should be typed? Or should it always use the inferred type?
        Hide
        Stephanie Van Dyk added a comment -

        Here's the quick assist. A few points:

        This assist is not intelligent about lists - it just assigns one name to the list and selects this new name so the user can change it if desired. If this is a highly desired feature, perhaps this choice can be re-evaluated.

        This fix makes a reasonable guess of a name based on the statement.

        Additionally, this assist is not available for statements that are return statements - either explicitly or implicitly.

        In terms of the key-binding, I sunk a few hours into it but I had no luck with it. Since the end of term is approaching, I wanted to share what I have.

        Finally, this fix is dependent on a helper function contained in the patch attached to GRE1346.

        Stephanie

        Show
        Stephanie Van Dyk added a comment - Here's the quick assist. A few points: This assist is not intelligent about lists - it just assigns one name to the list and selects this new name so the user can change it if desired. If this is a highly desired feature, perhaps this choice can be re-evaluated. This fix makes a reasonable guess of a name based on the statement. Additionally, this assist is not available for statements that are return statements - either explicitly or implicitly. In terms of the key-binding, I sunk a few hours into it but I had no luck with it. Since the end of term is approaching, I wanted to share what I have. Finally, this fix is dependent on a helper function contained in the patch attached to GRE1346. Stephanie
        Hide
        Andrew Eisenberg added a comment -

        I finally got a chance to look at your patch (apologies, still on vacation, so I must do it during nap time). This looks quite good, but there are some comments:

        1. The test suite looks good and it passes for me.
        2. The class comments on the AssignStatementToNewLocalProposal and AssignStatementToNewLocalRefactoring have not been updated.
        3. For list and map literals, I'd rather see the name of the extracted variable be list and map respectively. Currently, it is temp.
        4. The missing functionality is fine, but you should raise new issues for the following:
          1. The key-binding issue
          2. Lack of extraction at return statements
          3. Type inferencing of the extracted variable type
          4. Linked editing mode for the extracted variable name and its type (see the same quick assist in Java).
        Show
        Andrew Eisenberg added a comment - I finally got a chance to look at your patch (apologies, still on vacation, so I must do it during nap time). This looks quite good, but there are some comments: The test suite looks good and it passes for me. The class comments on the AssignStatementToNewLocalProposal and AssignStatementToNewLocalRefactoring have not been updated. For list and map literals, I'd rather see the name of the extracted variable be list and map respectively. Currently, it is temp . The missing functionality is fine, but you should raise new issues for the following: The key-binding issue Lack of extraction at return statements Type inferencing of the extracted variable type Linked editing mode for the extracted variable name and its type (see the same quick assist in Java).
        Hide
        Andrew Eisenberg added a comment -

        I am committing our current patch. So, can you make sure that any changes you make are based on top of head?

        Show
        Andrew Eisenberg added a comment - I am committing our current patch. So, can you make sure that any changes you make are based on top of head?
        Hide
        Stephanie Van Dyk added a comment -

        Thanks, Andrew!

        Here's another patch. It addresses the following issues:

        • comments in AssignStatementToNewLocalProposal and AssignStatementToNewLocalRefactoring are fixed
        • guessed names for lists and maps are more intelligent (list and map, as requested)

        About the return statement thing: doing so changes the meaning of the code in a fairly insidious way - because of Groovy's implicit return statement, a method will suddenly return void that perhaps had a more meaningful return value. Are we sure this is something we want to do? The Java version ignores return statements. I could go either way on this.

        As for type-inferencing, linked editing mode and the key-binding, I will address those shortly. Seperate issues for each?

        Thanks,
        Stephanie

        Show
        Stephanie Van Dyk added a comment - Thanks, Andrew! Here's another patch. It addresses the following issues: comments in AssignStatementToNewLocalProposal and AssignStatementToNewLocalRefactoring are fixed guessed names for lists and maps are more intelligent (list and map, as requested) About the return statement thing: doing so changes the meaning of the code in a fairly insidious way - because of Groovy's implicit return statement, a method will suddenly return void that perhaps had a more meaningful return value. Are we sure this is something we want to do? The Java version ignores return statements. I could go either way on this. As for type-inferencing, linked editing mode and the key-binding, I will address those shortly. Seperate issues for each? Thanks, Stephanie
        Hide
        Andrew Eisenberg added a comment -

        Resolving this issue. The final patch looks good.

        Please remember to raise the issues for the uncompleted work.

        Show
        Andrew Eisenberg added a comment - Resolving this issue. The final patch looks good. Please remember to raise the issues for the uncompleted work.

          People

          • Assignee:
            Stephanie Van Dyk
            Reporter:
            Bob Tiernay
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: