technomancy / slamhound

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

:require :rename is not supported #68

Closed seancorfield closed 10 years ago

seancorfield commented 10 years ago

We use :rename in a handful of places to create better names for functions. Slamhound gets confused by this. Not sure whether this is even possible to fix or just a limitation with looking up names? Example:

        [image-resizer.rotate :as rotate
         :refer [rotate-90-counter-clockwise-fn rotate-270-counter-clockwise-fn]
         :rename {rotate-90-counter-clockwise-fn rotate-right rotate-270-counter-clockwise-fn rotate-left}]

Slamhound says:

java.lang.Exception: Couldn't resolve rotate-left, got as far as {:alias {clojure.string str, clojure.java.io io}, :import #{javax.imageio.plugins.jpeg.JPEGImageWriteParam javax.imageio.ImageIO javax.imageio.stream.FileImageOutputStream javax.imageio.IIOImage}, :refer {worldsingles.data.core #{execute}, date-clj #{subtract today}, worldsingles.data.ws #{worldsingles-db}, clojure.java.jdbc #{query}}, :old {:load nil, :exclude {}, :xrefer #{}, :require #{}, :refer-all #{}, :verbose #{}, :rename {}, :alias {image-resizer.rotate rotate, clojure.string str, clojure.java.io io}, :reload #{}, :reload-all #{}, :gen-class nil, :import #{javax.imageio.plugins.jpeg.JPEGImageWriteParam javax.imageio.ImageIO javax.imageio.stream.FileImageOutputStream javax.imageio.IIOImage}, :refer {worldsingles.data.ws #{worldsingles-db}, image-resizer.rotate #{rotate-90-counter-clockwise-fn rotate-270-counter-clockwise-fn}, clojure.java.jdbc #{query}, date-clj #{subtract today}, worldsingles.data.core #{execute get-by-id}}}, :meta nil, :name worldsingles.admin.photos} regrow.clj:384 slam.hound.regrow/regrow hound.clj:13 slam.hound/reconstruct /Developer/workspace/worldsingles/ws/model/clojure/worldsingles/src/worldsingles/admin/photos.clj:1 worldsingles.admin.photos/eval47051

guns commented 10 years ago

:)

I've been waiting for this moment: fc7e887c80140299dd34d9b34b5e38996088e0e5

I'll get this hooked up today; the hardest part will actually be getting these long libspecs to print attractively.

Thanks!

seancorfield commented 10 years ago

Sorry :)

We use this in eight files, mostly to rename clojure.string functions so we don't have to use a namespace alias / prefix (yeah, counter-intuitive... I know) but there are cases like the one above where the original name is just so horrible we want to avoid it completely.

guns commented 10 years ago

Renames are a bit of a gray area in the ns macro; the docstrings for ns and require don't mention it at all. It's only documented in the refer docs (and therefore implicitly by use). Of course, all of these functions are really just frontends for load-lib, so everything pretty much works everywhere.

I suspect, however, that if I can't implement this :rename support cleanly, @technomancy might NAK on this feature.

BTW, you say you are using Light Table with Slamhound. Is there an official plugin for this? I'd love to link to it in the README.

seancorfield commented 10 years ago

LightTable plugin: https://github.com/chadhq/slamhound-lt

FWIW, I just removed most of our :renames where they didn't add much to the code. We can live without it if it's too much of a pain to implement (but some libraries just use really long, sucky names!).

technomancy commented 10 years ago

Yeah, part of the original philosophy behind slamhound is to use automation to encourage regularity in ns declarations, so stuff like renaming string vars is definitely not a sufficient use case.

Renaming functions with bad names that you don't have control over is more of a grey area; I probably wouldn't add support for it myself, but I wouldn't turn it down if someone else wanted to take a crack at it and could come up with something reasonable. =)

guns commented 10 years ago

Okay, I've implemented rename support in the rename-support branch. Here's a diff against master:

https://github.com/technomancy/slamhound/compare/rename-support

As you can see, it's almost entirely a clean accretion of code. The :rename path of the regrow routine is ONLY taken if a missing reference matches a renamed var in the original ns map (searching for possible renames is otherwise impossible).

If it works for you, and @technomancy thinks it's okay, we'll merge it in!

seancorfield commented 10 years ago

I tested it on the file that caused me to open this issue and it worked beautifully (it removed the :as alias on that required namespace since we weren't using it - so it cleaned things up too).

Thank you for your swift response to this!

technomancy commented 10 years ago

Nice work! Let's bring it in.

seancorfield commented 10 years ago

Thank you for releasing 1.5.3. Running it across all our code now to see what it picks up.

seancorfield commented 10 years ago

Finding some issues around handling of :import for which I'll open separate tickets but otherwise it's looking good and finding us some nice cleanups.