Adds enhancements to organize imports working

[wpopielarski]

Contains some enhancements for imports organizing

[ghprb-bot]

Refer to this link for build results (access rights to CI server needed):

https://jenkins.scala-ide.org:8496/jenkins/job/ghprb-scala-refactoring-validator/299/ https://jenkins.scala-ide.org:8496/jenkins/job/ghprb-scala-refactoring-2.10.x-sbt/129/ https://jenkins.scala-ide.org:8496/jenkins/job/ghprb-scala-refactoring-2.11.x-sbt/149/ Test PASSed.

[mlangc]

Hmm, at least for me, the implementation still adds

import scala.tools.<root>.scala.tools.refactoring.implementations.oimports.{ TreeToolbox => treeToolbox }

when organizing imports on scala.tools.refactoring.implementations.OrganizeImports. Can you reproduce this? Or do you want to handle this in another PR (I would be fine with that!)?

Apart from that this looks quite good to me, although I have to confess that my understanding of the code is rather limited. Can you maybe add some basic documentation to IsNotInImports, with one example where apply(tested) should return true and one example where it should return false?

[wpopielarski]

Hi Matthias, unfortunately I cannot reproduce it. I really don't get why in your case TreeToolbox is renamed. I'm going to add some doc to IsNotInImport

2016-04-11 22:07 GMT+02:00 Matthias Langer notifications@github.com:

Hmm, at least for me, the implementation still adds

import scala.tools..scala.tools.refactoring.implementations.oimports.{ TreeToolbox => treeToolbox }

when organizing imports on scala.tools.refactoring.implementations.OrganizeImports. Can you reproduce this? Or do you want to handle this in another PR (I would be fine with that!)?

Apart from that this looks quite good to me, although I have to confess that my understanding of the code is rather limited. Can you maybe add some basic documentation to IsNotInImports, with one example where apply(tested) should return true and one example where it should return false?

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/scala-ide/scala-refactoring/pull/158#issuecomment-208533416

[mlangc]

Strange, here are the settings I'm working with:

Did you try with similar ones?

[wpopielarski]

ok, I have set 'Preserve only wildcards; one import statement per importee otherwise' but I check all of them. Thanks for your eye

2016-04-12 21:35 GMT+02:00 Matthias Langer notifications@github.com:

Strange, here are the settings I'm working with:

[image: organize-imports-prefs] https://cloud.githubusercontent.com/assets/7326646/14473021/61d352d0-00f6-11e6-9016-47ccd9154837.png

Did you try with similar ones?

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/scala-ide/scala-refactoring/pull/158#issuecomment-209070150

[wpopielarski]

I see now, it is not working for first two options. I look at it.

2016-04-13 10:11 GMT+02:00 Wieslaw Popielarski wpopielarski@gmail.com:

ok, I have set 'Preserve only wildcards; one import statement per importee otherwise' but I check all of them. Thanks for your eye

2016-04-12 21:35 GMT+02:00 Matthias Langer notifications@github.com:

Strange, here are the settings I'm working with:

[image: organize-imports-prefs] https://cloud.githubusercontent.com/assets/7326646/14473021/61d352d0-00f6-11e6-9016-47ccd9154837.png

Did you try with similar ones?

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/scala-ide/scala-refactoring/pull/158#issuecomment-209070150

[wpopielarski]

Sorry for that but I cannot write good tests for this part. I need to look deeper on it in some future.

[ghprb-bot]

Refer to this link for build results (access rights to CI server needed):

https://jenkins.scala-ide.org:8496/jenkins/job/ghprb-scala-refactoring-validator/300/ https://jenkins.scala-ide.org:8496/jenkins/job/ghprb-scala-refactoring-2.11.x-sbt/150/ https://jenkins.scala-ide.org:8496/jenkins/job/ghprb-scala-refactoring-2.10.x-sbt/130/ Test PASSed.

[ghprb-bot]

Refer to this link for build results (access rights to CI server needed):

https://jenkins.scala-ide.org:8496/jenkins/job/ghprb-scala-refactoring-validator/301/ https://jenkins.scala-ide.org:8496/jenkins/job/ghprb-scala-refactoring-2.10.x-sbt/131/ https://jenkins.scala-ide.org:8496/jenkins/job/ghprb-scala-refactoring-2.11.x-sbt/151/ Test PASSed.

[wpopielarski]

@mlangc generally I have a problem with this piece of CompilationUnitDependencies:

            case imp @ Import(qualifier, selector)  => {
              if (inScopeForLocalImports) {
                if (!qualifier.symbol.isLocal && !qualifier.symbol.isVal) {
                  if (isRelativeToTopLevelImports(qualifier) || isRelativeToEnclosingPackage(qualifier)) {
                    fakeSelectTree(qualifier.tpe, qualifier.symbol, qualifier) match {
                      case select: Select => addToResult(select)
                      case _ => ()
                    }
                  }
                }

in my opinion ideally we could limit it to imports found in the top of package def. I don't see the reason why we should collect all imports and expose them to the package scope. But this impacts tests.

[ghprb-bot]

Test PASSed.

[mlangc]

@wpopielarski I'm not sure if I understand you correctly: Do you want to drop the isRelativeToEnclosingPackage(..) in the if above?

[mlangc]

Well, here is another example that bit me today at work (the code depends on "com.typesafe.play" %% "play-json" % "2.5.2" - I ran into another bug when attempting to get rid of the dependency on play) where "Organize Imports" breaks:

package com.github.mlangc.experiments

case class SomeCaseClass(x: Int)

object BasicJsonFormats {
  import play.api.libs.json._

  implicit val someClassJsonFormat = Json.format[SomeCaseClass]
}

case class Bug2(x: SomeCaseClass)

object Bug2 {
  import BasicJsonFormats._
  import play.api.libs.json._

  implicit val bug2JsonFormat = Json.format[Bug2]
}

Note that you can also find this example here: https://github.com/mlangc/scala-ide-experiments/tree/non-tree-print-org-imports-class

[wpopielarski]

@mlangc , I'm not sure about just dropping isRelativeToEnclosingPackage(..) but looks like it needs update and little redesign... problem with 'eye of sauron' should be moved to separate pr I guess. this text is inserted by tree printer.

[ghprb-bot]

Test PASSed.

[ghprb-bot]

Test PASSed.

[mlangc]

Can you rewrite dfc57bb5f700949f277e1ecce00e3e2c06044938 so that it doesn't include hs_err_pid7760.log?

[mlangc]

Did you have a look at this Json.format related bug by the way?

[wpopielarski]

not yet, unanswered means in progress

[mlangc]

As for isRelativeToEnclosingPackage(...): I can't say much about this without diving deeper into the code myself, although I added this check myself some time ago if I remember correctly. If you think that this would help you, I can do that in the coming days.

[ghprb-bot]

Test PASSed.

[wpopielarski]

@mlangc so the problem with Play isn't so easy at the moment. implicit val bug2JsonFormat = Json.format[Bug2] is translated to ValDef tree. This tree in its rhs contains TypeApply tree TypeApply tree has got set NonemptyAttachments in its rawatt The rawatt has got field all of type Set and this set contains such 3 elements:

• scala.tools.nsc.typechecker.StdAttachments.MacroExpanderAttachment
• scala.tools.nsc.typechecker.StdAttachments.MacroExpansionAttachment
• scala.tools.nsc.typechecker.StdAttachments.SuppressMacroExpansionAttachment Looks that most interesting is the second element. Its field expanded (of type tree Typed) contains field expr.expr of type Apply which in its innards keeps reference to com.github.mlangc.experiments.SomeCaseClass.

So it needs investigation

[wpopielarski]

digging deeper: expanded contains in fun:

play.api.libs.functional.syntax.`package`.toInvariantFunctorOps[play.api.libs.json.OFormat,
 com.github.mlangc.experiments.SomeCaseClass](play.api.libs.json.JsPath.\("s").format[com.github.mlangc.experiments.SomeCaseClass](BasicJsonFormats.someClassJsonFormat))(json.this.OFormat.invariantFunctorOFormat).inmap[com.github.mlangc.experiments.Bug2]

and args

List({
  ((s: com.github.mlangc.experiments.SomeCaseClass) => Bug2.apply(s))
}, play.api.libs.functional.syntax.`package`.unlift[com.github.mlangc.experiments.Bug2, com.github.mlangc.experiments.SomeCaseClass]({
  ((s: com.github.mlangc.experiments.Bug2) => Bug2.unapply(s))
}))

and expandee (of TypeApply) in fun play.api.libs.json.Json.format and in args List(com.github.mlangc.experiments.Bug2)

[mlangc]

@wpopielarski Do you want to handle that in another PR (I guess it's not a regression)?

[ghprb-bot]

Test FAILed.

[wpopielarski]

2.10 is down. MacroExpansion... has changed during transition to 2.11. I'll take it tomorrow. @mlangc if you suffer from insomnia try this guy 895d48c

[mlangc]

895d48cca1aad32a182c505ca267d9f139367cf5 looks fine to me! After taking care of 2.10, could you try to add a unit test for this problem? It would be our first test containing a Macro that I know of, and definitely worth some effort!

[wpopielarski]

say it got it, I try :)

2016-04-22 6:53 GMT+02:00 Matthias Langer notifications@github.com:

895d48c https://github.com/scala-ide/scala-refactoring/commit/895d48cca1aad32a182c505ca267d9f139367cf5 looks fine to me; after taking care of 2.10, could you try to add a unit test for this problem? It would be our first test containing a Macro that I know of, and definitely worth some effort!

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/scala-ide/scala-refactoring/pull/158#issuecomment-213256195

[ghprb-bot]

Test PASSed.

[wpopielarski]

@mlangc @sschaef I try to write a test for macro (or let call it 'play') scenario. Not sure if I succeed today. It looks that much easier would be to make it in scala-ide.

[ghprb-bot]

Test FAILed.

[ghprb-bot]

Test FAILed.

[wpopielarski]

@mlangc @sschaef there is a test defined but ignored. Last problem is with code of this test valid for 2.11 only. I fix it tomorrow so be ready to accept I hope and release this PR :)

[ghprb-bot]

Test FAILed.

[ghprb-bot]

Test PASSed.

[ghprb-bot]

Test PASSed.

[mlangc]

A part from the very minor remark above, this looks very good to merge to me! Keep up the good work!

[kiritsuku]

LGTM. @wpopielarski once you resolved Matthias' last remark I'll merge.

[ghprb-bot]

Test PASSed.

[wpopielarski]

Simon, ripe to be taken

[kiritsuku]

Let's be able to test it over the weekend. :ship: