Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deftask list syntax #574

Closed
aaronlahey opened this issue Jan 30, 2017 · 14 comments
Closed

deftask list syntax #574

aaronlahey opened this issue Jan 30, 2017 · 14 comments

Comments

@aaronlahey
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

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 👍

@aaronlahey
Copy link
Author

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
Copy link
Contributor

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

@DonyorM
Copy link
Contributor

DonyorM commented Apr 30, 2017

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
Copy link
Contributor

arichiardi commented Apr 30, 2017

I think this still needs a PR for the error message

@DonyorM
Copy link
Contributor

DonyorM commented Apr 30, 2017

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
Copy link
Contributor

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

@DonyorM
Copy link
Contributor

DonyorM commented May 1, 2017

@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
Copy link
Contributor

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
Copy link
Contributor

DonyorM commented May 1, 2017

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
Copy link
Contributor

DonyorM commented Jun 17, 2017

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

@alandipert
Copy link
Contributor

@DonyorM yes, I like it, thanks.

@alandipert
Copy link
Contributor

Resolved via #627

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants