[Fix #1776] Add new customization variable cider-test-defining-names.
#1906
Conversation
| @@ -59,6 +59,12 @@ | |||
| :group 'cider-test | |||
| :package-version '(cider . "0.9.0")) | |||
|
|
|||
| (defcustom cider-test-defining-names '("deftest" "defspec") | |||
| "Macro names that define individual tests." | |||
There was a problem hiding this comment.
I think you should expand the description here - if I didn't know what this was about I'd never figure it out. :-)
| @@ -59,6 +59,12 @@ | |||
| :group 'cider-test | |||
| :package-version '(cider . "0.9.0")) | |||
|
|
|||
| (defcustom cider-test-defining-names '("deftest" "defspec") | |||
| "Macro names that define individual tests." | |||
| :type 'list | |||
There was a problem hiding this comment.
The actual type should be '(repeat string).
| @@ -59,6 +59,12 @@ | |||
| :group 'cider-test | |||
| :package-version '(cider . "0.9.0")) | |||
|
|
|||
| (defcustom cider-test-defining-names '("deftest" "defspec") | |||
There was a problem hiding this comment.
I'd prefer cider-test-defining-forms/macros. Names is a bit vague I think.
|
I'm obviously fine with simple solutions. :-) This should definitely be added to the manual as well - with more extensive explanations, otherwise few people are going to figure out the possibility of customization even exists. |
|
Sorry about closing the PR - wrong button. :-) |
…efining-forms`. This fix is strictly simpler than that suggested in clojure-emacs#1776 (comment). It has the advantage of being much more robust. I partially implemented the suggested fix, and witnessed the following two issues. First, full "macroexpand" does not leave a recognizable `deftest` form; it generally leaves a `(def test-... (fn [] ...))` sexp. That implies that a recursive macroexpansion would be required, with each step of the recursion checking for a recognized form. Second, even recognizing such a form is tricky, because the expansion may refer to deftest in an aliased namespace (e.g., `t/deftest` or `clojure.test/deftest` rather than bare `deftest`). One could then try to identify possible namespaces using the current environment, but this is getting complicated. Therefore, I think it better to have the user configure their environment to help them solve this problem. (And, of course, future work can implement the full macroexpansion approach.)
|
@bbatsov thanks for such a fast and helpful review. I force pushed a new patch since this rewrote the entire original. Also, +1 to CIDER's great contributing checklist -- one of the best I've used on github. So far, a great contribution experience \o/ |
Happy to hear this! :-) Thanks for your contribution! |
This fix is strictly simpler than that suggested in
#1776 (comment). It
has the advantage of being much more robust.
I partially implemented the suggested fix, and witnessed the following two
issues. First, full "macroexpand" does not leave a recognizable
deftestform;it generally leaves a
(def test-... (fn [] ...))sexp. That implies that arecursive macroexpansion would be required, with each step of the recursion
checking for a recognized form. Second, even recognizing such a form is tricky,
because the expansion may refer to deftest in an aliased namespace (e.g.,
t/deftestorclojure.test/deftestrather than baredeftest). One couldthen try to identify possible namespaces using the current environment, but this
is getting complicated.
Therefore, I think it better to have the user configure their environment to
help them solve this problem. (And, of course, future work can implement the
full macroexpansion approach.)
Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):
make test)M-x checkdocwarningsThanks!