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

add a warning when the repl tasks :eval option is passed a string #666

Merged
merged 1 commit into from
Jan 9, 2018

Conversation

martinklepsch
Copy link
Member

@martinklepsch martinklepsch commented Dec 12, 2017

Currently this works:

boot repl -c -e '(println "hello")'

but this doesn't despite being the obvious "translation" of the above.

(deftask connect
  []
  (repl :client true
        :eval "(println \"hello\")"))

What's expected in this case is a quoted form; a string is just evaluated as a string essentially not doing anything:

(deftask connect
  []
  (repl :client true
        :eval '(println "hello")))

The change in this PR prints a warning if a user passes a string instead of a quoted form in Clojure code / build.boot:

When passing :eval to the repl task in your build.boot, use a quoted form instead of a string

I considered just using read-string but a warning seems fair and might also educate the user about the differences when passing edn options via the CLI vs. Clojure code.

Considered adding a Changelog entry but didn't really seem interesting enough.

Copy link
Contributor

@arichiardi arichiardi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with the warning here

@martinklepsch martinklepsch force-pushed the repl-eval-string-warning branch 3 times, most recently from 0525c16 to 9c6eb73 Compare December 14, 2017 17:04
@martinklepsch
Copy link
Member Author

Thanks for the review @arichiardi!

I think ultimately we should consider a form option type that will only accept Clojure forms a la (init) and not accept anything else (i.e. as in this case: edn).

@martinklepsch martinklepsch mentioned this pull request Dec 15, 2017
3 tasks
@martinklepsch martinklepsch force-pushed the repl-eval-string-warning branch 2 times, most recently from 1bc4b57 to ca1c37a Compare January 9, 2018 17:08
When users pass a string to the repl's `eval` option on the CLI this works fine:

    boot repl --eval "(println \"foo\")"

if they do the same in their build.boot or other Clojure code
the direct translation won't work:

    (repl :eval "(println \"foo\")")

The issue here is that the passed value is interpreted as edn and a string is a valid edn
value. This commit introduces a warning whenever the users passes a string to the :eval
option from Clojure code.

This issue probably affects other tasks making me think that there should be some kind
of `form` type for task options which are expected to be executable Clojure code.
@martinklepsch martinklepsch merged commit 180ed5b into master Jan 9, 2018
@martinklepsch martinklepsch deleted the repl-eval-string-warning branch January 9, 2018 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants