boot-clj / boot

Build tooling for Clojure.
https://boot-clj.github.io/
Eclipse Public License 1.0
1.75k stars 180 forks source link

deftask list syntax #574

Closed aaronlahey closed 7 years ago

aaronlahey commented 7 years ago

I prefer to define my single-arity functions the same way I do multi-arity functions. Example:

(defn foo
  ([]
   (println "foo")))

I did this out of habit on the deftask macro...

(deftask package
  "Example task..."
  ([]
   (comp (jar)
         (target))))

...and got this error which was a bit confusing:

 clojure.lang.ExceptionInfo: Wrong number of args (1) passed to: cli/argspec->cli-argspec
    data: {:file
           "/var/folders/mf/4t4byj3s7xncnj8lpr_1tzf40000gn/T/boot.user2925592664059683955.clj",
           :line 15}
clojure.lang.ArityException: Wrong number of args (1) passed to: cli/argspec->cli-argspec
               ...
boot.main/-main/fn   main.clj: 196
   boot.main/-main   main.clj: 196
               ...
  boot.App.runBoot   App.java: 399
     boot.App.main   App.java: 488
               ...
         Boot.main  Boot.java: 258

Is there any interest in making deftask a bit more like defn (supporting a list containing the bindings/forms) or adding a more helpful error message?

arichiardi commented 7 years ago

My 2c: it is interesting but I guess also kind of hard cause if you open that Pandora's box then you also need to parse multi-arity task options.

And what happens if I use I have one deftask with two arities with different sets of options?

I guess that would be the most difficult case to handle correctly and frankly I see no big benefit in it. Of course I might be totally wrong here πŸ˜€πŸ˜€

RadicalZephyr commented 7 years ago

I think adding a more helpful error message would be great, but I don't think adding support for multiple arities makes any sense because boot tasks are already fundamentally multi-arity because all options are passed as keyword-style args and most should probably be optional or have sane-defaults.

Also, along with what @arichiardi points out, there are other things (task-options!, help text generation that would be vastly complicated by such a change without any real benefit.

Better error messages are always a good thing though :+1:

aaronlahey commented 7 years ago

Thanks for the input @arichiardi and @RadicalZephyr!

For the record, I wasn't suggesting adding support for multiple arities, just adding support for...

(deftask foo
  ([]
   (comp (a) (b))))

...in addition to...

(deftask foo
  []
  (comp (a) (b)))

...and perhaps throwing an exception stating "deftask does not support multiple arities" for something like...

(deftask foo
  ([]
   (comp (a) (b)))
  ([f foo VAL "..."]
   (comp (a) (b) (c))))

You two know the internals of boot way better than I do and are in a much better position to decide if the added complexity is worth it, though. If not, an error message (and perhaps a docstring change to show usage) would be very helpful. πŸ‘

alandipert commented 7 years ago

:+1: for a better error message here, a PR would be most welcome. Thanks for the suggestions.

DonyorM commented 7 years ago

Boot is now giving a new error message:

java.lang.IllegalArgumentException: Parameter declaration should be a vector: ([] (comp (jar) (target)))
        clojure.lang.ExceptionInfo: Parameter declaration should be a vector: ([] (comp (jar) (target)))

Has this been fixed or do we still want a more descriptive error?

arichiardi commented 7 years ago

I think this still needs a PR for the error message

DonyorM commented 7 years ago

Does this seem like a decent error message? "Multiple arity format not supported for clifns and tasks. Use single arity format."

I hesitate to give an example, because this will apply to all clifns, not just tasks, so using a specific example could confuse newcomers.

arichiardi commented 7 years ago

What is a clifn? Do you mean a task called from the command line? In that case we can drop I believe.

DonyorM commented 7 years ago

@arichiardi deftask calls the defclifn macro, which apparently applies to a larger group than simply tasks. This is what I assume, I'm not real familiar with the code base yet. So a task is a clifn I believe.

arichiardi commented 7 years ago

yes defclifn I thinks comes from an external package but I guess in boot it is used only for tasks. I am pretty sure but for a last confirmation we'd better ask @micha or @alandipert

DonyorM commented 7 years ago

It comes from the namespace boot.cli, line 251. There seems to be a clojure.tools.cli, but it appears to be a different thing.

However, it's still very possible clifns are only used by deftask. If that's the case, is this a better error message:

"Multiple arity format not supported for tasks. Use single arity format. Ex (deftask build [x y ...] ;commands)"

DonyorM commented 7 years ago

@micha @alandipert Does the above error message look good?

alandipert commented 7 years ago

@DonyorM yes, I like it, thanks.

alandipert commented 7 years ago

Resolved via #627