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.

Sign up for GitHub

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

Jump to bottom

Use a better error when a cljs repl form cannot be found #2307

Merged
merged 1 commit into from
Jun 6, 2018
Merged

Use a better error when a cljs repl form cannot be found #2307

merged 1 commit into from
Jun 6, 2018

Conversation

arichiardi
Copy link
Contributor

@arichiardi arichiardi commented Jun 5, 2018 •
edited
Loading

Without this fix cider throws an error but it is not a very clear one.

Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

  • The commits are consistent with our [contribution guidelines][1]

  • All tests are passing (make test)

  • All code passes the linter (make lint) which is based on elisp-lint and includes

    • This fails with exit = 2 but I don't think it due to my change.

  • You've updated the [changelog][3] (if adding/changing user-visible functionality)

@arichiardi arichiardi changed the title Return nil when the cljs REPL form cannot be found Handle nil case when the cljs repl form cannot be found Jun 5, 2018
@arichiardi arichiardi changed the title Handle nil case when the cljs repl form cannot be found Handle nil when the cljs repl form cannot be found Jun 5, 2018
@arichiardi arichiardi force-pushed the fix-nil-repl-form branch 2 times, most recently from 73eca6b to 7a06850 Compare June 5, 2018 00:13
@bbatsov
Copy link
Member

bbatsov commented Jun 5, 2018

Hmm, I'm not sure that suppressing the error is a good idea. Probably it should just be made more meaningful using user-error. After your changes simply nothing's going to happen if people mistyped/misconfigured something, which is going to be pretty confusing.

@arichiardi
Copy link
Contributor Author

Uhm True, will try a better fix

bbatsov
bbatsov reviewed Jun 5, 2018
View reviewed changes
cider.el Outdated
@@ -755,7 +755,7 @@ you're working on."

(defun cider-cljs-repl-form (repl-type)
"Get the cljs REPL form for REPL-TYPE."
(let ((repl-form (cadr (seq-find
(when-let ((repl-form (cadr (seq-find
Copy link
Member

Choose a reason for hiding this comment

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

I guess you can just use if-let here and show an user-error in the else branch. Something like No ClojureScript REPL type x found. Please check ...

@arichiardi arichiardi changed the title Handle nil when the cljs repl form cannot be found Use a better error when a cljs repl form cannot be found Jun 5, 2018
@arichiardi
Copy link
Contributor Author

arichiardi commented Jun 5, 2018 •
edited
Loading

@bbatsov done 😄

@arichiardi arichiardi force-pushed the fix-nil-repl-form branch 2 times, most recently from 54505a5 to 9b58752 Compare June 5, 2018 19:20
bbatsov
bbatsov reviewed Jun 5, 2018
View reviewed changes
cider.el Outdated
(if (symbolp repl-form)
(funcall repl-form)
repl-form)
(user-error "No ClojureScript REPL type x found. Please make sure that `cider-cljs-repl-types` has an entry for `cljs-repl-type`")))
Copy link
Member

Choose a reason for hiding this comment

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

:D You should have replaced x with some interpolation of repl-type. Same for cljs-repl-type which is not een a param for this function. 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah got you! Let me google Elisp interpolations :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I thought Elisp had a construct for interpolations :D

bbatsov
bbatsov reviewed Jun 6, 2018
View reviewed changes
cider.el Outdated
(if (symbolp repl-form)
(funcall repl-form)
repl-form)))
(if-let ((repl-form (cadr (seq-find
Copy link
Member

Choose a reason for hiding this comment

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

I see this in the build log cider.el:756:30:Error: (repl-form (cadr (seq-find (lambda (entry) (eq (car entry) repl-type)) cider-cljs-repl-types)))' is a malformed function. If think by mistake someone made if-let` work with just one set of parens and the form was immediately deprecated afterwards in Emacs 26.1. See how we're using this elsewhere in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh ok I actually did check the docstring for compatibility and it was saying both format are supported. Anyways will change.

bbatsov
bbatsov reviewed Jun 6, 2018
View reviewed changes
cider.el Outdated
(if (symbolp repl-form)
(funcall repl-form)
repl-form)
(user-error (format "No ClojureScript REPL type %s found. Please make sure that `cider-cljs-repl-types` has an entry for it" repl-type))))
Copy link
Member

Choose a reason for hiding this comment

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

I think user-error does format implicitly inside. You can check the docstring.

@arichiardi
Use a better error when a cljs repl form cannot be found
e258a60 
Without this fix cider throws an error but it is not a very clear one.
@bbatsov bbatsov merged commit 49034dc into clojure-emacs:master Jun 6, 2018
@bbatsov
Copy link
Member

bbatsov commented Jun 6, 2018

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbatsov bbatsov bbatsov left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@arichiardi @bbatsov