technomancy / slamhound

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

Feature - add clojure.test as [clojure.test :refer :all] #29

Closed AlexBaranosky closed 11 years ago

AlexBaranosky commented 11 years ago

Require-refer-all clojure.test when regrowing the namespaces:

[clojure.test :refer :all]
technomancy commented 11 years ago

Yeah, that sounds good to me.

AlexBaranosky commented 11 years ago

This is done right?

guns commented 11 years ago

It would be nice if this were optional, defaulting either way.

EDIT

A little justification:

My own attraction to slamhound is its ability to remove unused requires and imports from my namespaces. The use of :refer :all with clojure.test runs counter to this desire, especially since most projects I see typically only use [deftest is testing].

guns commented 11 years ago

@AlexBaranosky

Any comment RE: https://github.com/technomancy/slamhound/issues/29#issuecomment-18937847

Would love to stay away from mass refer.

AlexBaranosky commented 11 years ago

I have no objections to making it optional... Ask @technomancy though: he's the project owner, and may prefer less options, and a more opinionated approach.

technomancy commented 11 years ago

Yeah, either way is fine by me. Fewer special cases can be good.

guns commented 11 years ago

@AlexBaranosky: may prefer … a more opinionated approach

@technomancy: Yeah, either way is fine by me

This is not very opinionated! :) So I think I will have to convince AlexBaranosky.

:refer :all is always used purely out of convenience. In my mind, however, Slamhound partly exists to allow lazy programmers to mindlessly use vars from random namespaces without suffering the consequences of pouring dozens of unused vars into a namespace.

Why go through the trouble otherwise? We could just :refer :all by default, unless there is a var conflict. This would be convenient!

Then there is the trouble of why we are singling out clojure.test. Many people also use (:require [midje.sweet :refer :all]):

https://github.com/search?q=midje+%3Arefer+%3Aall&type=Code&ref=searchresults

I don't think treating a single test namespace differently from other popular test namespaces is justifiable, even if one is official.

Also, some people may consider :refer :all to be conventional in test namespaces, but I found it quite annoying that Slamhound took my carefully curated list of :refer […] and replaced it with a mass import.

I started using Slamhound to avoid this exact situation, so I found this special case unpleasantly surprising.

So, I propose that we revert this change and go back to the same behaviour for all namespaces. What do you think @AlexBaranosky?

technomancy commented 11 years ago

I'll be more opinionated then: fewer special cases is good. =) I agree that clojure.test shouldn't be a special case. The ability to add :refer :all namespaces on a per-project basis might be good, but I haven't figured out a clean way to pass configuration options to slamhound.

guns commented 11 years ago

The ability to add :refer :all namespaces on a per-project basis might be good

I think that's a great solution.

but I haven't figured out a clean way to pass configuration options to slamhound

A no-configuration way is to simply honor existing :refer :all imports and move on (but not :use!). When the user decides he'd like to tidy his namespace, he can simply remove that expression and re-run Slamhound.

technomancy commented 11 years ago
guns commented 11 years ago

lol, I'll take this as a directive then.

PR in a ~couple of days. :)

guns commented 11 years ago

Fixed by #50

refer: https://github.com/technomancy/slamhound/issues/29#issuecomment-22086914