search

technomancy / slamhound

Slamhound rips your namespace form apart and reconstructs it.
Other
473 stars 38 forks source link

Confuses imports when multiple classes match #73

Closed seancorfield closed 10 years ago

seancorfield commented 10 years ago

We import (javax.xml.rpc.holders StringHolder) in one of our files. Slamhound rewrites this to an import of StringHolder from a CORBA package that is on the class path (but not imported anywhere).

Slamhound should prefer original imports if present (much like you had to do for :rename).

guns commented 10 years ago

This should have been addressed in the 1.5.0 release… could you please tell me the artifact that contains javax.xml.rpc.holders.StringHolder?

seancorfield commented 10 years ago

It's in axis-jaxrpc-1.4.jar.

Here's the ns declaration before:

(ns worldsingles.iov
  (:require [worldsingles.environment :refer [my-settings]])
  (import   java.net.URL
            javax.xml.rpc.holders.StringHolder
            (com.iesnare.www.dra.api.GetEvidenceDetails GetEvidenceDetailsResponseEvidence_detailsEvidence)
            (com.iesnare.www.dra.api.CheckTransactionDetails CheckTransactionDetailsResponseDetailsDetail
             holders.CheckTransactionDetailsResponseDetailsHolder)
            com.newrelic.api.agent.Trace))

and here's it after Slamhound:

(ns worldsingles.iov
  (:require [worldsingles.environment :refer [my-settings]])
  (:import (com.iesnare.www.dra.api.CheckTransactionDetails CheckTransactionDetailsResponseDetailsDetail)
           (com.iesnare.www.dra.api.CheckTransactionDetails.holders CheckTransactionDetailsResponseDetailsHolder)
           (com.iesnare.www.dra.api.GetEvidenceDetails GetEvidenceDetailsResponseEvidence_detailsEvidence)
           (java.net URL)
           (org.omg.CORBA StringHolder)))

Curiously, at this point if I fix the StringHolder type and run Slamhound again, it no longer gets confused. In other words, if I start from this:

(ns worldsingles.iov
  (:require [worldsingles.environment :refer [my-settings]])
  (:import (com.iesnare.www.dra.api.CheckTransactionDetails CheckTransactionDetailsResponseDetailsDetail)
           (com.iesnare.www.dra.api.CheckTransactionDetails.holders CheckTransactionDetailsResponseDetailsHolder)
           (com.iesnare.www.dra.api.GetEvidenceDetails GetEvidenceDetailsResponseEvidence_detailsEvidence)
           (com.newrelic.api.agent Trace)
           (java.net URL)
           (javax.xml.rpc.holders StringHolder)))

Then then the only thing Slamhound does is to remove the NewRelic Trace import (known issue in another ticket).

Perhaps the extra work Slamhound does to split out the holders package affects something in the algorithm?

guns commented 10 years ago

Thank you for the descriptive reply; I think I have a good idea of what's going on here. Be back in a few.

guns commented 10 years ago

Aha! Your ns :import statement reads:

(import …)

Instead of:

(:import …)

I suppose it wouldn't be hard for Slamhound to make up for the ns macro's extreme permissiveness…

seancorfield commented 10 years ago

Oh good catch! I hadn't noticed that difference! Thank you!