martinklepsch / tenzing

⚡️ Clojurescript application template using Boot
Eclipse Public License 1.0
403 stars 39 forks source link

rename test task to avoid collision #48

Open instilled opened 8 years ago

instilled commented 8 years ago

Martin,

First of all great work with tenzing. Awesome bootstrap for cljs with boot!

Just skimmed the generated build.boot and noticed your comment about the name collision:

;;; This prevents a name collision WARNING between the test task and
;;; clojure.core/test, a function that nobody really uses or cares
;;; about.
(ns-unmap 'boot.user 'test)
(deftask test []
  (comp (testing)
        (test-cljs :js-env :phantom
                   :exit?  true)))

Did you know about replace-task! in boot? With the following you can easily redefine an existing task:

(replace-task!
 [t test] (fn [& xs] (comp (testing) (test-cljs :js-env :phantom :exit?  true))))

;; or if you still want to invoke the original, redefined task something along the lines:
;;(replace-task! [t test] (fn [& xs] (comp (testing) (apply t xs))))

Anyways, Thansk for the great work!

crisptrutski commented 8 years ago

Hey Martin Fabio (woah, too many festive beverages)

Thanks for the suggestion, I hadn't seen this replace-task! macro before - it's pretty groovy.

One thing to note is that clojure.core/test is not actually a task at all. It's a fairly obscure function that could make some sense from the REPL, but has nothing to do with regular unit test runners, or boot tasks.

It seems to me this replace-task! macro is just sugar for decorating tasks, and a bit clumsy for defining something novel. You're not actually generating the CLI metadata, or using the required binding variable.

The warning and unmap mechanism you quote above is fairly "orthodox", being taken from the official boot-test repo.

On the other hand, I agree that it's a fairly technical, scare-quotey piece of code to place in such a prominent position for users, guessing that was your motivation opening this. While I don't think replace-task! is the improvement we'd want to apply, I'm interested in a better solution.

Perhaps 'boot.user should even come with this var stripped by default?

crisptrutski commented 8 years ago

Whoops, didn't mean to close! A clumsy trackpad fumble :stuck_out_tongue_closed_eyes:

martinklepsch commented 8 years ago

We could also rename the task. I often use test! as a task name to avoid collision.