search

technomancy / slamhound

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

Require heuristic works better; creates :require/:refer instead of :use/:only #18

Closed AlexBaranosky closed 11 years ago

AlexBaranosky commented 11 years ago

Looks like I got the require working, at least for the two test cases I had that were previously failing.

I'd like to address the last issue I have on my list, but don't know how. That is: Slamhound seems to miss vars in the syntax-quote part of a macro definition.

If you can see any flaws in the approach I took, or anything that could be cleaned up etc, let me know, and I'll get on them.

AlexBaranosky commented 11 years ago

This require/as solution opens up the possibility for more there to be more candidate namespaces. I'm working on having the disambiguate function prefer namespaces where the last piece of the namespace matches the missing alias (very much like the old require-as candidate logic).

technomancy commented 11 years ago

Can we get rid of *config*? I would like slamhound to emit namespaces in a single canonical format, and there's currently no way to propagate configuration from project.clj anyway since slamhound can be invoked from a variety of different tools.

AlexBaranosky commented 11 years ago

My only issue with it is I will just have to change the namespaces by hand afterward. But I'll revert the config change and we can maybe discuss he intention behind it.

AlexBaranosky commented 11 years ago

For the record, I ran this against a 1.5.0-RC1 app from work and it worked like a charm. I haven't tried it on our bigger, hairier app, because it isn't on 1.4 or 1.5 yet.

AlexBaranosky commented 11 years ago

What do you think of the way I sorted the require/as's on top and the require/refer's on the bottom? I was on the fence about that, or whether all the requires should just be sorted alphabetically, letting the as's and refer's fall where they may.

technomancy commented 11 years ago

Got another chance to review. The rest of the code looks great except for project.clj and a few over-80 column lines, but I can merge and fix those.

Sorting :requires with :as first followed by :refer is fine; I think it makes more sense that way.

Let's create a separate issue for the syntax-quote problem since it's not related to this change.

technomancy commented 11 years ago

Merged this in; thanks!

If you don't have immediate plans to fix the syntax-quote issue I will cut a release today, but if you want to do a bit more hacking on it I can wait.

AlexBaranosky commented 11 years ago

I've got no immediate plans for the syntax-quote issue currently.