Organize Imports Changes doesn't keep shortest path

Under previous versions of the plugin organize imports would not change this code at all:

package biz.cgta.oms.core.messages import biz.cgta.pint.trade.typedstrings.{ChainMessageId, ExchangeOrderId} import biz.cgta.pint.putil.ids.FirmId import biz.cgta.pint.trade.{Qty, Confirm} import biz.cgta.SPredef import biz.cgta.corrimano.util.CEnum import biz.cgta.oms.core.messages.GMConfirmBys.GMConfirmBy import biz.cgta.corrimano.io.serial.ser.{SerClass, CSerial} //////////////////////////////////////////////////////////////
// Some header ////////////////////////////////////////////////////////////// import SPredef._ import CSerial._


Under the new version the scala plugin transforms it to this:

import biz.cgta.pint.trade.typedstrings.{ChainMessageId, ExchangeOrderId} import biz.cgta.pint.putil.ids.FirmId import biz.cgta.pint.trade.{Confirm, Qty} import biz.cgta.SPredef import biz.cgta.corrimano.util.CEnum import biz.cgta.oms.core.messages.GMConfirmBys.GMConfirmBy import biz.cgta.corrimano.io.serial.ser.{CSerial, SerClass} ////////////////////////////////////////////////////////////// // Some header ////////////////////////////////////////////////////////////// import biz.cgta.SPredef._ import biz.cgta.corrimano.io.serial.ser.CSerial._


In these examples SPredef and CSerial are both objects.

There are a couple of issues here:
1. I would much prefer to leave the imports as alone like they are in the top code sample, I don't want fully qualified imports for SPredef / CSerial
2. Another bug is that biz.cgta.SPredef as well as CSerial (in import biz.cgta.corrimano.io.serial.ser.{CSerial, SerClass}) at the top is now redundant (and the plugin warns that it is an unused import)
3. Despite having sort imports lexicographically unchecked it reorderd Qty, Confirm to Confirm, Qty in the (import biz.cgta.pint.trade.{Confirm, Qty}) statement.

I have attached my settings in a screenshot because the post image button just shows a blank white box.

My guess is that the "Use the shortest path, when trying to import reference with already imported name" should be doing what I want here, but it doesn't
seem to play nicely with Add fully qualified imports

Like I said previously (before all the re-work to organize imports) this worked as expected.

We have other developers here using 14 and they see the same issue.



Attachment(s):
Screenshot - 11122014 - 01:49:47 PM.png
2 comments
Comment actions Permalink

Hi, Benjamin,

It's official position, that relative imports are not really good idea. It's good for text editors, where you have to manually import things, however it's less readable, when IDE imports everthing not fully qualified.

1. However you can setup such behaviour. You just need to uncheck "Add fully qualified imports".
2. That would reduce performance of optimize imports as we would need to run few iterations of optimize imports for every file, however in most cases single iteration is enough.
3. That's a bug, I just fixed it: https://github.com/JetBrains/intellij-scala/commit/e333d08885ab8ca2e3dc7899f885b0f9f6d4f728 It will be available in nightly builds or in 1.2, which will be released at the beginning of December.

Best regards,
Alexander Podkhalyuzin.

0
Comment actions Permalink

Hi Alexander,

Thank you for the speedy reply, Unfortunately, disabling "Add fully qualified imports" is not what I want to do. I agree they are bad form and should be discouraged.
What I am requesting is different. Once a symbol has a fully qualified import, then subsequent imports of that symbol should use that. This worked fine for the last 5+
years only in the recent refactor of organize imports did this break.

I believe that is what the "Use the shortest path, when trying to import reference with already imported name" setting is supposed to be doing.

To things simpler the pattern we follow is this:

Fully Qualified Imports
//File Header
Manual imports for dsls / implicits

Here are some real simple example of that is this

package biz.cgta.shapes

import biz.cgta.corrimano.io.serial.ser.CSerial
//////////////
//Ben Jackman 2014
/////////////

object Circle {
  //Import the Serialization DSL, which will dump several implicits into scope
  //Note CSerial has a fully qualified import at the top of the file already.
  import CSerial._
  implicit val ser = forCase(this.apply _)
}
case class Circle(r : Double)

object Square {

   //Optimizie import now ignores that (previously it respected that)
  //and will instead change this line and all others to the fully qualified import
  import CSerial._
  implicit val ser = forCase(this.apply _)

}

case class Square(s : Double)



Now to top this off the editor appears to be designed with the behaviour in mind.
because when I wrote out this example here is what happened:

0.) I made a new file in intellij and started with this file

package biz.cgta.shapes

//File Header


1.)  I added Circle

package biz.cgta.shapes

//File Header

object Circle {
}
case class Circle(r : Double)


2.) Inside of object Circle I added CSerial:

object Circle {
  import CSerial._
}
case class Circle(r : Double)



3.) Intellij prompts me that CSerial is an unknown symbol and offers to add the import for it,
which I accept. Intellij adds the fully qualified import where I think it belongs, which is at the top
of the file, making the file look like this:

package biz.cgta.shapes

import biz.cgta.corrimano.io.serial.ser.CSerial
//File Header

object Circle {

import CSerial._

}

case class Circle(r : Double)


4.) I run optimize imports which messes up the file to look like this:

package biz.cgta.shapes

import biz.cgta.corrimano.io.serial.ser.CSerial
//File Header

object Circle {

import biz.cgta.corrimano.io.serial.ser.CSerial._

}

case class Circle(r : Double)


After I run optimize imports in step 4 the file should remain unchanged as it had in all previous versions of intellij up until optimize imports was changed.

So, again this all used to work fine previously, I don't want to use relative imports, I just want to the optimize imports to respect the a symbol is already
imported once at the top with a fully qualified import rather than making every single import below become fully qualified. I believe that is the purpose
behind this setting: "Use the shortest path, when trying to import reference with already imported name" which is in already in intellij but which doesn't
seem to work.

Thanks,
Ben

0

Please sign in to leave a comment.