Details

    • Type: New Feature New Feature
    • Status: Resolved Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.7.0.Release
    • Component/s: Refactoring
    • Labels:
      None
    • Number of attachments :
      5

      Description

      We want to add a refactoring that is similar to Eclipse's Convert Local Variable to Field (i.e. promote local variable.)

      For example,

      class Foo {
        def bar(a, b) {def c = 0}
      }
      

      becomes

      class Foo {
        def c
        def bar(a, b) {c = 0}
      }
      
      1. ExtractLocalToFieldRefactoring.patch
        54 kB
        Daniel Phan
      2. local-to-field.patch
        54 kB
        Daniel Phan
      3. local-to-field-2.patch
        41 kB
        Daniel Phan
      4. Test.groovy
        0.5 kB
        Daniel Phan
      1. screenshot-1.jpg
        34 kB

        Activity

        Hide
        Andrew Eisenberg added a comment -

        One thing to look at is how reusable the existing convert to field refactoring wizard is. For example, in the screenshot, could we have a section for where to initialize the field? Also, being able to specify an access parameter is nice.

        Show
        Andrew Eisenberg added a comment - One thing to look at is how reusable the existing convert to field refactoring wizard is. For example, in the screenshot, could we have a section for where to initialize the field? Also, being able to specify an access parameter is nice.
        Hide
        Daniel Phan added a comment -

        I've attached what I have so far. It compiles and displays the first page of the wizard, but doesn't actually perform the refactor yet.

        Show
        Daniel Phan added a comment - I've attached what I have so far. It compiles and displays the first page of the wizard, but doesn't actually perform the refactor yet.
        Hide
        Andrew Eisenberg added a comment -

        Thanks for the initial patch. This is a good start. I haven't run the code, but I eyeballed it. Two comments so far:

        1. It looks like you copied the messages in check initial/final conditions from Extract constant refactoring. Make sure to update all of the messages to something reasonable for convert local to field.
        2. I do see the copied code from Extract constant. Best thing to do for now is to mark each copied section with a comment. And once the functionality is there we can decide what to do. 3 possibilities really, and it's hard to say what the best option is until we see how much copied code there really is):
          1. Leave as is
          2. Extract copied code to a utility class and convert to static methods
          3. Extract an abstract common super class for Extract constant and convert to field. Put the common code in the super class.

        With the copied code, it's best for now to mark the copied segments and change as little as possible inside of them so that when it comes time to refactor, then we will have the easiest time doing it.

        Show
        Andrew Eisenberg added a comment - Thanks for the initial patch. This is a good start. I haven't run the code, but I eyeballed it. Two comments so far: It looks like you copied the messages in check initial/final conditions from Extract constant refactoring. Make sure to update all of the messages to something reasonable for convert local to field. I do see the copied code from Extract constant. Best thing to do for now is to mark each copied section with a comment. And once the functionality is there we can decide what to do. 3 possibilities really, and it's hard to say what the best option is until we see how much copied code there really is): Leave as is Extract copied code to a utility class and convert to static methods Extract an abstract common super class for Extract constant and convert to field. Put the common code in the super class. With the copied code, it's best for now to mark the copied segments and change as little as possible inside of them so that when it comes time to refactor, then we will have the easiest time doing it.
        Hide
        Andrew Eisenberg added a comment -

        Two more comments:

        1. Do not include the .classpath changes in the path. They should not be committed.
        2. And for the changes to FindSurroundingNode, I need to be very careful with those changes so that they don't break existing code. It looks like your changes to not cause testing failures, but I'll have a deeper look later. Also, keep this in a separate patch for now.
        Show
        Andrew Eisenberg added a comment - Two more comments: Do not include the .classpath changes in the path. They should not be committed. And for the changes to FindSurroundingNode , I need to be very careful with those changes so that they don't break existing code. It looks like your changes to not cause testing failures, but I'll have a deeper look later. Also, keep this in a separate patch for now.
        Hide
        Daniel Phan added a comment -

        Here is a file that I've been using for manual testing and debugging that contains some interesting cases (at the bottom).

        Show
        Daniel Phan added a comment - Here is a file that I've been using for manual testing and debugging that contains some interesting cases (at the bottom).
        Hide
        Daniel Phan added a comment -

        Still needs to be done:

        • Test.groovy cases added to test suite
        • Existing tests in suite updated to match correct behaviour
        • Fix all the FIXMEs
        Show
        Daniel Phan added a comment - Still needs to be done: Test.groovy cases added to test suite Existing tests in suite updated to match correct behaviour Fix all the FIXMEs
        Hide
        Andrew Eisenberg added a comment -

        Thanks for the update. This is on the right track. Some comments:

        1. Add some comments to the tricky parts of your code, like in getDeclarationExpression, createFieldText, and any other place that is potentially tricky.
        2. There is no concept of final in Groovy, so you can ignore that part of the refactoring.
        3. getRefactoringDescriptor is not something we are supporting right now, so you don't need to implement the method. This method is supposed to enable scripting your refactoring changes, but it doesn't really work (even in Java), so there's not much point in putting much effort into it.
        4. Rather than use the existing PromoteTempWizard, it would be better to sub-class it and disable functionality that you will not be supporting (eg- final, initialize in field, initialize in constructor, etc). Or if you prefer, you can copy the entire class and place it in the refactoring plugin and then make edits to simplify the wizard. See here for how to copy code: http://docs.codehaus.org/display/GROOVY/Groovy-Eclipse+coding+conventions

        I have not actually applied the patch or run the tests. Let me know if you think you are ready for me to do that.

        Show
        Andrew Eisenberg added a comment - Thanks for the update. This is on the right track. Some comments: Add some comments to the tricky parts of your code, like in getDeclarationExpression , createFieldText , and any other place that is potentially tricky. There is no concept of final in Groovy, so you can ignore that part of the refactoring. getRefactoringDescriptor is not something we are supporting right now, so you don't need to implement the method. This method is supposed to enable scripting your refactoring changes, but it doesn't really work (even in Java), so there's not much point in putting much effort into it. Rather than use the existing PromoteTempWizard , it would be better to sub-class it and disable functionality that you will not be supporting (eg- final, initialize in field, initialize in constructor, etc). Or if you prefer, you can copy the entire class and place it in the refactoring plugin and then make edits to simplify the wizard. See here for how to copy code: http://docs.codehaus.org/display/GROOVY/Groovy-Eclipse+coding+conventions I have not actually applied the patch or run the tests. Let me know if you think you are ready for me to do that.
        Hide
        Daniel Phan added a comment -

        The feature and tests are complete. Please take a look at the attached patch and add your feedback.

        Show
        Daniel Phan added a comment - The feature and tests are complete. Please take a look at the attached patch and add your feedback.
        Hide
        Andrew Eisenberg added a comment -

        Thanks. This is looking really close. A few small comments:

        1. I am getting 3 test failures. Are they failing for you? It looks like the Local variable is not being properly found and so the extraction cannot occur. If they are passing for you, or you need me to take a deeper look, I can.
          1. testForLoop
          2. testPostfix
          3. testPrefix
        2. You have a bunch of TODO statements in the extract refactoring class. It is fine to leave these sections unimplemented, but there should not be a TODO comment there. If you plan on implementing this functionality later, change the comment to FIXDP (your initials) and include a comment as to what needs to happen. Or if you don't plan on implementing, then the same comment explaining what is left undone, but without the FIXDP is fine. In either case, a link to a relevant jira is good.
        3. After you have done these two things, you should raise a new jira explaining what is left to do (even if you don't think you will get to it or if it is even worth doing ever).
        Show
        Andrew Eisenberg added a comment - Thanks. This is looking really close. A few small comments: I am getting 3 test failures. Are they failing for you? It looks like the Local variable is not being properly found and so the extraction cannot occur. If they are passing for you, or you need me to take a deeper look, I can. testForLoop testPostfix testPrefix You have a bunch of TODO statements in the extract refactoring class. It is fine to leave these sections unimplemented, but there should not be a TODO comment there. If you plan on implementing this functionality later, change the comment to FIXDP (your initials) and include a comment as to what needs to happen. Or if you don't plan on implementing, then the same comment explaining what is left undone, but without the FIXDP is fine. In either case, a link to a relevant jira is good. After you have done these two things, you should raise a new jira explaining what is left to do (even if you don't think you will get to it or if it is even worth doing ever).
        Hide
        Andrew Eisenberg added a comment -

        I dug a little deeper and it looks like the failing tests are due to the problems that you discovered with the FindSurroundingNode class. I made the fixes for that class and reran the tests. They all passed.

        I am now writing some more tests for FindSurroundingNode to ensure that the changes don't break anything. I don't think we ever raised a jira for that.

        Show
        Andrew Eisenberg added a comment - I dug a little deeper and it looks like the failing tests are due to the problems that you discovered with the FindSurroundingNode class. I made the fixes for that class and reran the tests. They all passed. I am now writing some more tests for FindSurroundingNode to ensure that the changes don't break anything. I don't think we ever raised a jira for that.
        Hide
        Andrew Eisenberg added a comment -

        I fixed the problem with FindSurroundingNode. See GRECLIPSE-1424. All of your tests are passing for me now.

        I will commit the current patch and then can you submit a new patch to update the comments? This way your next patch can be significantly smaller.

        Show
        Andrew Eisenberg added a comment - I fixed the problem with FindSurroundingNode . See GRECLIPSE-1424 . All of your tests are passing for me now. I will commit the current patch and then can you submit a new patch to update the comments? This way your next patch can be significantly smaller.
        Hide
        Daniel Phan added a comment -

        The improvements have been listed in GRECLIPSE-1436.

        Show
        Daniel Phan added a comment - The improvements have been listed in GRECLIPSE-1436 .
        Hide
        Andrew Eisenberg added a comment -

        Resolving. Added a link to the bug report in the code.

        Show
        Andrew Eisenberg added a comment - Resolving. Added a link to the bug report in the code.
        Hide
        Andrew Eisenberg added a comment -
        Show
        Andrew Eisenberg added a comment - Link to pull request. https://github.com/groovy/groovy-eclipse/pull/1

          People

          • Assignee:
            Daniel Phan
            Reporter:
            Daniel Phan
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: