Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Extending the refactoring support by integrating new refactorings from t... #111

Merged
merged 7 commits into from

5 participants

@michih57

...he refactoring library.

The new integrated refactorings are:

  • Method signature refactorings:

    • Change parameter order (within parameter lists)
    • Split parameter lists
    • Merge parameter lists
  • Extract trait
    Extracts class members (val/var/def) to a new trait

  • Move constructor to companion object
    Redirects calls constructor calls to a newly generated apply-method of the companion object.
    If necessary, the companion object is generated as well.

  • Source generators:

    • Generate hashCode and equals
    • Introduce ProductN trait Generates methods to implement ProductN for selected class parameters. This includes generation of hashCode and equals.
...a/tools/eclipse/refactoring/IndexedRefactorings.scala
((26 lines not shown))
+
+ val project = sourcefile.project
+
+ val refactoring: MultiStageRefactoring with GlobalIndexes with Indexed
+
+ /**
+ * A cleanup handler, will later be set by the refactoring
+ * to remove all loaded compilation units from the compiler.
+ */
+ var cleanup = () => ()
+
+ override def checkInitialConditions(pm: IProgressMonitor): RefactoringStatus = {
+ val status = super.checkInitialConditions(pm)
+
+ if (!status.hasError) {
+ // the hints parameter is not used, this can slow down the refactoring considerably!
@dragos Owner
dragos added a note

This indeed worries me: A full project index for a non-trivial project can take minutes. While this can be overlooked in an individual refactoring, in a framework class that is intended to be used by many other refactorings this is very bad.

You need to at least defer this decision to subclasses of IndexedIdeRefactoring, so each can decide to be slow on their own. :)

Ah, I forgot about Mirko's comment about the hints parameter. Anyway, now I created a hook for subclasses to provide hints and MoveConstructorToCompanionObjectAction is already using it. For refactorings that operate on the method signature I don't see a way to come up with useful hints, as one has to track all usages of the refactored method, so they will be slow...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dotta dotta commented on the diff
...la/tools/eclipse/refactoring/ExtractTraitAction.scala
((7 lines not shown))
+import scala.tools.refactoring.common.TextChange
+import scala.tools.refactoring.implementations.ExtractTrait
+
+import org.eclipse.core.resources.IFile
+import org.eclipse.core.runtime.IProgressMonitor
+import org.eclipse.jdt.internal.corext.refactoring.nls.changes.CreateFileChange
+import org.eclipse.ltk.core.refactoring.{Change => EclipseChange}
+import org.eclipse.ltk.core.refactoring.CompositeChange
+
+import javaelements.ScalaSourceFile
+
+/**
+ * The ExtractTrait refactoring extracts members (vals, vars and defs) of a class
+ * or trait to a new trait.
+ * The original class/trait will automatically extend the extracted trait and the
+ * extracted trait will have a self-type annotation for the original class/trait.
@dotta Owner
dotta added a note

Why always put the self-type? I understand that the extracted trait may need access to some member of the original class, but that's not always the case. Wouldn't it be possible to be smarter about this? (If you think this is a valid thing to do, you don't have to fix it now, but maybe file an enhancement ticket on the scala-refactoring issue tracker)

@michih57
michih57 added a note

That's actually a 'bug' in the comment, the self type is only added when necessary, i.e. when the trait accesses class members. I'll correct the comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...la/tools/eclipse/refactoring/ExtractTraitAction.scala
((19 lines not shown))
+ * The ExtractTrait refactoring extracts members (vals, vars and defs) of a class
+ * or trait to a new trait.
+ * The original class/trait will automatically extend the extracted trait and the
+ * extracted trait will have a self-type annotation for the original class/trait.
+ */
+class ExtractTraitAction extends RefactoringAction {
+
+ def createRefactoring(selectionStart: Int, selectionEnd: Int, file: ScalaSourceFile) = new ExtractTraitScalaIdeRefactoring(selectionStart, selectionEnd, file)
+
+ class ExtractTraitScalaIdeRefactoring(start: Int, end: Int, file: ScalaSourceFile)
+ extends ScalaIdeRefactoring("Extract trait", file, start, end) with ExtractTraitConfigurationPageGenerator {
+
+ val refactoring = withCompiler { c =>
+ new ExtractTrait with GlobalIndexes {
+ val global = c
+ var index = GlobalIndex(Nil)
@dotta Owner
dotta added a note

GlobalIndex(Nil) should be changed in NoIndex EmptyIndex

@dotta Owner
dotta added a note

Does index really need to be a var? I'm asking because turning it into a val seem to result in no compilation errors...

@michih57
michih57 added a note

I changed it to val index = EmptyIndex

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
.../ChangeParameterOrderConfigurationPageGenerator.scala
((31 lines not shown))
+ override val secondBtnText = "Down"
+
+ // If a parameter is selected we
+ // - enable the up button when the selected parameter is not
+ // the first one in its parameter list and disable it otherwise
+ // - enable the down button when the selected parameter is not
+ // the last one in its parameter list and disable it otherwise
+ override def setBtnStatesForParameter(
+ param: ValDef,
+ paramsWithSeparators: List[Either[ValDef, ParamListSeparator]],
+ upBtn: Button,
+ downBtn: Button) {
+ val isLast = isLastInParamList(param, paramsWithSeparators)
+ val isFirst = isFirstInParamList(param, paramsWithSeparators)
+
+ downBtn.setEnabled(!(isLast && isFirst) && !isLast)
@dotta Owner
dotta added a note

isn't !isLast enough to express the condition?

@michih57
michih57 added a note

That's right. I made a special case for parameter lists of size one, missing that for those the general condition implies the specific one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
.../ChangeParameterOrderConfigurationPageGenerator.scala
((32 lines not shown))
+
+ // If a parameter is selected we
+ // - enable the up button when the selected parameter is not
+ // the first one in its parameter list and disable it otherwise
+ // - enable the down button when the selected parameter is not
+ // the last one in its parameter list and disable it otherwise
+ override def setBtnStatesForParameter(
+ param: ValDef,
+ paramsWithSeparators: List[Either[ValDef, ParamListSeparator]],
+ upBtn: Button,
+ downBtn: Button) {
+ val isLast = isLastInParamList(param, paramsWithSeparators)
+ val isFirst = isFirstInParamList(param, paramsWithSeparators)
+
+ downBtn.setEnabled(!(isLast && isFirst) && !isLast)
+ upBtn.setEnabled(!(isLast && isFirst) && !isFirst)
@dotta Owner
dotta added a note

Same comment as above, but in this case !isFirst should be enough.

@michih57
michih57 added a note

Right again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dotta
Owner

I was trying out the new refactoring, great job Michael! I really love them!

A few things I noticed:

  • Generating hashcode on an empty class resulted in the following (a bit surprising :)) implementation injected:
override def hashCode() = {
  val prime = 41
  1 // why 1 :)
}
  • I love that generating equals also creates canEqual. Maybe you could also make the enclosing class/trait to mix-in Equals. WDYT?
@dotta
Owner

Also, I tried extract trait on the following class

class Foo {
  def foo {}
}

And I got back an exception:

java.util.NoSuchElementException: head of empty list
    at scala.collection.immutable.Nil$.head(List.scala:402)
    at scala.collection.immutable.Nil$.head(List.scala:399)
    at scala.tools.refactoring.implementations.ExtractTrait.refactorClass(ExtractTrait.scala:112)
    at scala.tools.refactoring.implementations.ExtractTrait.perform(ExtractTrait.scala:83)
    at scala.tools.refactoring.implementations.ExtractTrait.perform(ExtractTrait.scala:13)
    at scala.tools.eclipse.refactoring.ScalaIdeRefactoring$$anonfun$5$$anonfun$apply$7.apply(ScalaIdeRefactoring.scala:153)
    at scala.tools.eclipse.refactoring.ScalaIdeRefactoring$$anonfun$5$$anonfun$apply$7.apply(ScalaIdeRefactoring.scala:152)
    at scala.tools.nsc.util.InterruptReq.liftedTree1$1(InterruptReq.scala:20)
    at scala.tools.nsc.util.InterruptReq.execute(InterruptReq.scala:19)
    at scala.tools.nsc.interactive.Global.pollForWork(Global.scala:330)
    at scala.tools.nsc.interactive.PresentationCompilerThread.run(PresentationCompilerThread.scala:22)
...la/tools/eclipse/refactoring/ExtractTraitAction.scala
((47 lines not shown))
+
+ val configPage = mkConfigPage(
+ extractableMembers,
+ members => selectedMembers = members,
+ name => traitName = name)
+
+ override def getPages = configPage::Nil
+
+ override def createChange(pm: IProgressMonitor): CompositeChange = {
+ val (textChanges, newFileChanges) = {
+ performRefactoring().foldRight((List[TextChange](), List[NewFileChange]())) {
+ case (change: TextChange, (textChanges, newFiles)) =>
+ (change :: textChanges, newFiles)
+ case (change: NewFileChange, (textChanges, newFilesChanges)) =>
+ (textChanges, change :: newFilesChanges)
+ }
@dotta Owner
dotta added a note

I'm always a bit suspicious at foldRight, because it can lead to StackOverflowException. I'm not sure what's the data set size to expect for this scenario, but if possible it would be better to change this.

@michih57
michih57 added a note

I changed it to foldLeft

@dotta Owner
dotta added a note

great! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...la/tools/eclipse/refactoring/ExtractTraitAction.scala
((58 lines not shown))
+ case (change: TextChange, (textChanges, newFiles)) =>
+ (change :: textChanges, newFiles)
+ case (change: NewFileChange, (textChanges, newFilesChanges)) =>
+ (textChanges, change :: newFilesChanges)
+ }
+ }
+
+ // Create a new file for the extracted trait
+ val fileChange: Option[EclipseChange] = newFileChanges.headOption.map(newFile => {
+ val pkg = file.getCompilationUnit.getParent
+ val path = pkg.getPath.append(traitName + ".scala")
+ new CreateFileChange(path, newFile.text, file.getResource.asInstanceOf[IFile].getCharset)
+ })
+
+ val changes = fileChange.toList ::: scalaChangesToEclipseChanges(textChanges).toList
+ val compositeChange = new CompositeChange(getName)
@dotta Owner
dotta added a note

Why not new CompositeChange(getName, changes), and delete the next two lines.

@michih57
michih57 added a note

I wasn't aware of this constructor, looks better now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...a/tools/eclipse/refactoring/IndexedRefactorings.scala
((3 lines not shown))
+import scala.tools.eclipse.javaelements.ScalaSourceFile
+import scala.tools.refactoring.analysis.GlobalIndexes
+import scala.tools.refactoring.MultiStageRefactoring
+
+import org.eclipse.core.runtime.IProgressMonitor
+import org.eclipse.ltk.core.refactoring.RefactoringStatus
+
+/**
+ * Helper trait that adds an index variable to a refactoring.
+ * Needed to be able to factor out common functionality of refactorings
+ * that need an index of the full project.
+ * @see IndexedIdeRefactoring
+ */
+trait Indexed {
+ this: GlobalIndexes =>
+ var index = GlobalIndex(Nil)
@dotta Owner
dotta added a note

Also here it may be better to use EmptyIndex

@michih57
michih57 added a note

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dotta
Owner

Looks good to me!

I have a couple of suggestions related to coding convetions:

  • Always declare the return type of non-private members.

  • Improve encapsulation of members. For instance, in ExtractTraitAction it looks like selectedMembers, traitName, configPage and extractableMembers can all be declared as private (likley, there are other places where this should also be done). IMO, it's a good practice to reduce visiblity of members that don't need to be exposed, and it improves readability.

Code conventions are unfortunately something we did not formalize yet, so take the above points as what I believe to be good style. And please, feel free (you and/or the community) to criticize the above suggestions if they don't make sense.

@michih57

First of all thanks for all the suggestions and I'm glad you like the new refactorings :)
Now to the comments I haven't answered yet:

hashCode and equals

  • hasCode value
override def hashCode() = {
  val prime = 41
  1 // why 1 :)
}

Let's call this an implementation artifact in the degraded case without any class parameters. The reason is that a fold is done over the selected class parameters, and 1 is the seed of this fold.

  • Mixing in Equals is a good idea IMO, so I implemented it ;)

Extract trait exception

You found a bug here, as it didn't work in the default package so far. This is fixed now.

Coding conventions

I tried to follow your suggestions, should be much better now.

@dotta
Owner

About the hashCode implementation, you did not manage to convince me :) The convoluted degraded case should just return prime or 1(for uniformity I'd inject prime, but it's your decision. The important thing is that no useless code is generated).

Did you push your changes? (this pull-request doesn't seem to include your last modifications)

By the way, you definitely earned my personal awesome contributor badge (it entitles you to get a few free beer the first time we meet ;)). Keep up the good work!

@michih57

I'm honored :)

I did push the changes, the pull request now has 3 commits. Some of the changes were in the refactoring library itself, maybe this is what you are missing?

About hashCode: I'll try to clean that up.

@michih57

Ok, now I got this:

override def hashCode() = {
  val prime = 41
  prime
}

Is this ok?

@dotta
Owner

Now I do see your additional commit, I'll have a look soon.

And the hashCode implementation is just what I was looking for :) good job!

@dotta
Owner

One more question, did you already push your changes in scala-refactoring? I'd like to have a look around with your latest fixes.

@michih57

Sorry, just pushed it now.

@dotta
Owner

No problem. I'll have a look at it tomorrow and give you feedback asap.

In the meanwhile, it would be great if you could:

1) Write some documentation for the new refactoring. You will have to fork the docs repo for this.
Add a new-refactoring folder here and describe the new features. Maybe have a look at Mirko's doc on Move Class/Trait/Object for inspiration :)

2) In the scala-ide website, we need a feature box to advertise the new refactoring. A feature box is basically an image with a short description. Look here for examples. You will have to fork the scala-ide.github.com repo for this.

I know, it is a bit of work, but doing this will highly increase discoverability of the new refactoring. And I'm hoping you do want the community to use these new awesome refactoring ;-)

@dotta
Owner

So, I've built a local version of the Scala IDE based on your branch for testing. I plan to use it all day tomorrow, but here are a couple of issues I've been seeing.

Create the following class:

class Foo {
  // put the cursor here and generate hashcode and equals
}

generating hashcode and equals results in the following:

// Between Foo and extends there are 2 spaces. It should be only 1.
class Foo  extends Equals {
  def canEqual(other: Any) = {
    other.isInstanceOf[Foo]
  }

  override def equals(other: Any) = {
    other match {
      case that: Foo => that.canEqual(Foo.this)
      case _ => false
    }
  }

  override def hashCode() = {
    val prime = 41
    prime
  }
}} // <-- There is one more closing bracket here.

One more thing. I believe both Generate hashcode and equals and Introduce ProductN trait should be moved to the Source menu (right click in the Scala Editor > Source).

Indeed, we also have a ticket for adding source generation refactoring: http://scala-ide-portfolio.assembla.com/spaces/scala-ide/tickets/1001018 (you may want to add a link to the mentioned ticket in your final commit message, something like Generate hashCode and equals (Re #1001018) would do it) //cc @misto

@michih57
  • Documentation/Website: I'll try to get that done soon

  • Bracket issue: that's (unfortunately) a known problem of the compiler. Mirko reported it here. I know it's not the best solution, but a simple newline at the end of the file should do the trick for now.

  • Extra space before extends: I forwarded this to Mirko, as the final source generation of a refactoring is strictly his domain ;)

  • GenerateHashcodeAndEquals and IntroduceProductNTrait to Source menu: I strongly agree here. I tried to figure out how to to that some time ago but I failed. Just noticed that this menu has become Scala-specific, I'll try it again. To speed this up: can you give me some pointers how to do that?

@dotta
Owner

Oh, I did not know about the position issue in scalac. I just voted it up, hope it helps :)

About extending the Source menu, it looks like it is going to be a bit of a hack, but it should be possible: http://stackoverflow.com/questions/7113380/how-to-extend-the-source-menu-in-eclipse-or-what-is-its-locationuri

@dotta
Owner

Source Generation Refactoring

Better handling of boundary scenario

class Foo extends Equals {
  def canEqual(that: Any) = false
}

generate hashcode and equals fails with the following exception

java.util.NoSuchElementException: Either.right.value on Left
    at scala.Either$RightProjection.get(Either.scala:451)
    at scala.tools.eclipse.refactoring.source.ClassParameterDrivenIdeRefactoring.classParams(ClassParameterDrivenIdeRefactoring.scala:28)
    at scala.tools.eclipse.refactoring.source.GenerateHashcodeAndEqualsAction$GenerateHashcodeAndEqualsScalaIdeRefactoring.<init>(GenerateHashcodeAndEqualsAction.scala:29)
    at scala.tools.eclipse.refactoring.source.GenerateHashcodeAndEqualsAction.createRefactoring(GenerateHashcodeAndEqualsAction.scala:17)
    at ...

Cleaner source generation

The generated source is a bit noisy. For instance

that.canEqual(Foo.this).&&(name.==(that.name)).&&(i.==(that.i))

Would be nicer as

that.canEqual(Foo.this) && name == that.name && i == that.i

But I guess this is something for Mirko, isn't it? :)

Merge Parameter Lists

Constructor's parameters

For both

class Foo
class Foo(s: String)(v: Int)

merge parameter lists fail with the following exception

java.util.NoSuchElementException: Either.right.value on Left
    at scala.Either$RightProjection.get(Either.scala:451)
    at scala.tools.eclipse.refactoring.method.MethodSignatureScalaIdeRefactoring.defdef(MethodSignatureIdeRefactoring.scala:32)
    at scala.tools.eclipse.refactoring.method.MethodSignatureScalaIdeRefactoring.configPage(MethodSignatureIdeRefactoring.scala:35)
    at scala.tools.eclipse.refactoring.method.MethodSignatureScalaIdeRefactoring.getPages(MethodSignatureIdeRefactoring.scala:39)
    at ...

I'm guessing the refactoring doesn't support constructor arguments. If this was an oversight, it would be nice to support it. If this is by design, then a dialog should be shown to the user saying that constructor's parameters are not supported.

Extract Trait

// A.scala
trait A {
 def foo(s: String)
}

// Foo.scala
class Foo extends A {
  override def foo(s: String) {}
}

Extract Trait on Foo with foo method selected, will perform the following, wrong, refactoring

//  Foo.scala
class Foo extends A { with Extracted // <-- notice here the opening bracket is misplaced
}

// Extracted.scala
trait Extracted {
  override def foo(s: String) {} // <-- there should be *no* override here
}

Expand Case Class Binding

case class Foo

fail with the following exception

java.util.NoSuchElementException: Either.right.value on Left
    at scala.Either$RightProjection.get(Either.scala:451)
    at scala.tools.eclipse.refactoring.ScalaIdeRefactoring$$anonfun$5$$anonfun$apply$7.apply(ScalaIdeRefactoring.scala:155)
    at scala.tools.eclipse.refactoring.ScalaIdeRefactoring$$anonfun$5$$anonfun$apply$7.apply(ScalaIdeRefactoring.scala:154)
    at scala.tools.nsc.util.InterruptReq.liftedTree1$1(InterruptReq.scala:20)
    at scala.tools.nsc.util.InterruptReq.execute(InterruptReq.scala:19)
    at scala.tools.nsc.interactive.Global.pollForWork(Global.scala:330)
    at scala.tools.nsc.interactive.PresentationCompilerThread.run(PresentationCompilerThread.scala:22)

I understand this is not how it is supposed to be used, but still, it should fail gracely (something like a Operation not supported on selected element dialog would be nice, IMO).

@dotta
Owner

Hey Michael, I finally found the time to test the refactoring. Above is some feedback for you :)

There are a few boundary cases that need to be handled, but overall it looks great!

@misto
Owner

@dotta Yes the formatting is my job, I've wanted to implement this for a long time but haven't had the time, but now that we generate such ugly code i should really fix it.

"Expand Case Class Binding" is also my fault :-)

Thanks a lot for the testing, Mirco!

@dotta
Owner

@misto I assumed "Expand Case Class Binding" was a new refactoring because I did not see documentation for it here :)

@michih57
  • Source generators: Failures because of the presence of an equality method (equals, canEqual or hashCode) are now properly reported in a dialog.

  • Method signature refactorings on constructors:
    I must admit that I never thought of including constructors in these refactorings, altough it makes perfect sense now ;)
    I'll have to take a deeper look into this, for now I've made them fail when a constructor is selected.

  • Extract trait:
    @misto can you take a look at the bracket issue?
    @dotta I hope to get that override issue fixed tomorrow. As far as I understand now, I can simply remove any existing override-modifier in the extracted trait, since the extracted trait doesn't extend anything?

Also a big thanks for the thorough testing!

@dotta
Owner
  • What do you mean by:

Failures because of the presence of an equality method (equals, canEqual or hashCode) are now properly reported in a dialog.

I think the right thing to do would be to ask the user if he wants to keep his current implementation or replace it with the generated one. Is that too much work?

  • Method signature refactorings on constructors: Awesome :)

  • Always removing override keyword in the extracted trait's methods seem like the right thing to do (I can't think of a failing case).

@dotta
Owner

@misto Awesome!

@michih57

I already fixed Extract trait and made the source generators more user-friendly (it is now possible to either keep or replace existing implementations of equals, canEqual and hashCode). However, I'm haven some build issues. I assume it's just me and maven, but I'll be gone for the next two weeks, so it will have to wait. Just wanted to report on some progress ;)

@dotta
Owner

I'll have a look right away. Let's try to merge this today :) (if possible, of course)

@dotta
Owner

On second thought, it's wiser not to rush this in :)

@michih57

I did a push now (both the refactoring library and the ide) so you can at least look at it ;)

@dotta
Owner

Your branch compiles now (you needed an up-to-date p2 repository for scala-refactoring, which I produced by building the Scala-IDE nightly).

  • Great job on moving the source generation refactoring in the Source menu item.

  • I tried generate hashcode and equals on this:

class Foo extends Equals {
  def canEqual(that: Any) = false
}

And got

class Foo extends Equals with Equals // <-- something is wrong here :)
  def canEqual(that: Any) = false

  override def equals(other: Any) = {
    other match {
      case that: Foo => that.canEqual(Foo.this)
      case _ => false
    }
  }

  override def hashCode() = {
    val prime = 41
    prime
  }
}

Not sure if that is expected (i.e., you already knew about it).

(Make sure to add all discussed code samples in the scala-refactoring regression suite, if you haven't already ;))

Let's discuss this more once you are back. Enjoy your time off (holidays?) :)

@michih57

Yep, holidays it was :)

I fixed the double occurrence of Equals.

Documentation is also already existing in the forked repositories. Should I issue pull requests for those too?

@dotta
Owner

Hi Michael, great to have you back!

About the doc, yes, please, create a pull request for the doc as well.

So, it's all good from your side (anything more you need to do before merging)? Did you add tests in scala-refactoring regression suite for all the discussed issues? (sorry for asking, but I just want to make sure that the useful work we have been doing in the past month won't be lost ;))

@michih57

Yes, as far as I can tell everything should be ready.
All tests that resulted from this discussion are included in the scala-refactoring test suite (just checked to be sure ;))
Also created pull requests for the documentation.

@dotta
Owner

Awesome. I'll try to have a look by tomorrow then, and if all is good I'll merge it! :)

@dotta
Owner

Oh, I just noticed the Jenkins BOT failed because of merge conflicts. Could you rebase your branch with the current HEAD (master) and then git -f push origin issue/extend-refactoring-support-1000960, where origin is the name of your remote (the pull-request will be automatically updated, and no comment will be lost)

@michih57

Did the rebase - hopefully correctly.

@dotta
Owner

I fear something is wrong. What did you rebase against? Did you remember to fetch changes from the Scala-IDE repo before rebasing?

Here is what I'd have done (assuming you are in issue/extend-refactoring-support-1000960 branch):

git fetch upstream 
git rebase -i upstream/master
(resolve conflicts)*

upstream is resolved to git://github.com/scala-ide/scala-ide.git

I just tried, and it looks like the final result should be the good one. You will have a few conflict in org.scala-ide.sdt.core/src/scala/tools/eclipse/refactoring/ScalaIdeRefactoring.scala, but they all seem easy enough to fix. Once you have rebased, git push -f your branch again, and hopefully it will be all good.

michih57 added some commits
@michih57 michih57 Extending the refactoring support by integrating new refactorings fro…
…m the refactoring library.

The new integrated refactorings are:

- Method signature refactorings:
  - Change parameter order (within parameter lists)
  - Split parameter lists
  - Merge parameter lists

- Extract trait
  Extracts class members (val/var/def) to a new trait

- Move constructor to companion object
  Redirects calls constructor calls to a newly generated apply-method of the companion object.
  If necessary, the companion object is generated as well.

- Source generators:
  - Generate hashCode and equals
  - Introduce ProductN trait
    Generates methods to implement ProductN for selected class parameters.
    This includes generation of hashCode and equals.
0f91849
@michih57 michih57 Added indexHints method to IndexedRefactoring to provide subclasses a…
… hook to speed up index building.
f7751e4
@michih57 michih57 Applied pull request suggestions by Mirco, fixed some index problems. 3db6b29
@michih57 michih57 Moved source generators to Source menu, properly handle refactoring p…
…reparation errors.

Generate hashCode and equals and Introduce ProductN trait now show up in the Source context menu. This is done programmatically in ScalaSourceFileEditor#editorContextMenuAboutToShow, so they are removed from plugin.xml.

When a refactoring fails in the preparation step this is now properly handled, the error message is presented in a dialog.
1efd575
@michih57 michih57 improved source generators
when generating hashCode and equals (and canEqual) implemenations and the class has already implementations for some of these methods, the user now can choose either to keep or replace his implementations.
15d4c9f
@michih57 michih57 improved message for existing equality methods in source generators 3906155
@michih57

Looks like I actually forgot the fetch. I'm quite confident I got it right this time ;)

@dotta
Owner

Yep, it looks good now, and I could successfully build your branch.

Though, this and the example in Extract Trait are still not producing the right transformation (the opening bracket is missing in both cases after the refactoring is performed, and the extracted trait has too many closing brackets :)).
In theory this should have been fixed in refactoring-#75, but I still see the issue with the latest scala-refactoring build. \cc @misto

@dotta
Owner

Alright, I had a few more style-related comments. As soon as you are done with them, I'm going to merge this in. It's really the last 100 meters ;-)

@michih57

Did the 100 meters ;)

I won't be able to complete the doc today, but I'll get it done tomorrow.

@dotta
Owner

Awesome! Let's ask the Mr. Jenkins to rebuild. If all goes fine, a merge will follow shortly ;)

PLS REBUILD ALL

@dotta dotta merged commit 1d9bead into scala-ide:master
@dotta
Owner

Champagne!

@dotta
Owner

Michael, could you please get in touch with Mirko and check what should be done wrt #111 (comment) (opening new tickets?)

@michih57

Cheers! :)
@dotta and @misto thank you both for all your support in this!

Concerning the issue: seems to work fine for me. Did you add any whitespace at the end of the file (could be the mentioned whitespace issue again)?

@dotta
Owner

Mmmm, let's see with tomorrow's nightly then :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 27, 2012
  1. @michih57

    Extending the refactoring support by integrating new refactorings fro…

    michih57 authored
    …m the refactoring library.
    
    The new integrated refactorings are:
    
    - Method signature refactorings:
      - Change parameter order (within parameter lists)
      - Split parameter lists
      - Merge parameter lists
    
    - Extract trait
      Extracts class members (val/var/def) to a new trait
    
    - Move constructor to companion object
      Redirects calls constructor calls to a newly generated apply-method of the companion object.
      If necessary, the companion object is generated as well.
    
    - Source generators:
      - Generate hashCode and equals
      - Introduce ProductN trait
        Generates methods to implement ProductN for selected class parameters.
        This includes generation of hashCode and equals.
  2. @michih57

    Added indexHints method to IndexedRefactoring to provide subclasses a…

    michih57 authored
    … hook to speed up index building.
  3. @michih57
  4. @michih57

    Moved source generators to Source menu, properly handle refactoring p…

    michih57 authored
    …reparation errors.
    
    Generate hashCode and equals and Introduce ProductN trait now show up in the Source context menu. This is done programmatically in ScalaSourceFileEditor#editorContextMenuAboutToShow, so they are removed from plugin.xml.
    
    When a refactoring fails in the preparation step this is now properly handled, the error message is presented in a dialog.
  5. @michih57

    improved source generators

    michih57 authored
    when generating hashCode and equals (and canEqual) implemenations and the class has already implementations for some of these methods, the user now can choose either to keep or replace his implementations.
  6. @michih57
Commits on Jun 28, 2012
  1. @michih57
Something went wrong with that request. Please try again.