implementation of cider-expected-ns

[aiba]

See clojure-emacs/clojure-mode#372

[bbatsov]

I think that just adding explicit shoulds will make this more readable. Our gain from dolist is pretty minimal right now.

[bbatsov]

Apart from my small remarks the changes look good. When you address them you should also squash the commits together and have the final commit follow our commit message guidelines.

[aiba]

I addressed the 2 comments, squashed the commits, and changed the commit message to match the guidelines.

[bbatsov]

Great! :-)

Guess now you can continue with moving the cljr functionality here (or you can wait for @benedekfazekas to do it). Depends on how fast you want to use this. :-)

[benedekfazekas]

haha I am happy either way. don't stop @aiba while you are at it tho :)

I guess removal from cljr should happen after 2.2.0 release as that will be compatible with 0.11 cider which won't have this functionality.

[bbatsov]

We can add the new version to cider and you can remove the old one afterwards. That'd be the most painless solution imo.

[benedekfazekas]

sorry if was not clear, that is what i meant. I only added that I would merge the removal from cljr after cljr 2.2.0 released as 2.2.0 will be meant to compatible with cider 0.11

[aiba]

Thanks for the help getting this into shape!

Currently clj-refactor has a feature, cljr--add-ns-if-blank-clj-file, which is great except that it calls clojure-insert-ns-form and therefore suffers from clojure-emacs/clojure-mode#372.

As a next step, I'd propose changing cljr--add-ns-if-blank-clj-file to call cider-expected-ns. After that, we could maybe remove clojure-insert-ns-form if nobody would miss it. (I wouldn't miss it.)

Or, @benedekfazekas are you suggesting that the insertion of namespace doesn't belong in either cljr or clojure-mode, and we should move it all to cider?

[bbatsov]

As a next step, I'd propose changing cljr--add-ns-if-blank-clj-file to call cider-expected-ns. After that, we could maybe remove clojure-insert-ns-form if nobody would miss it. (I wouldn't miss it.)

While this makes sense, we're actually going to move the feature completely to cider. Don't update cljr at all for now and don't update clojure-mode either (compatibility's a bitch).

Or, @benedekfazekas are you suggesting that the insertion of namespace doesn't belong in either cljr or clojure-mode, and we should move it all to cider?

Exactly. Just submit the relevant code to cider, don't touch the other projects for now. We'll do the cleanup there after cljr 2.2 is released eventually.

[benedekfazekas]

indeed. As @bbatsov said.

[aiba]

Sounds like a plan! I'll work on a PR for cider for cider--add-ns-if-blank-clj-file.

[Malabarba]

You may want to look into auto-insert-mode, which should already have some functions to make your life easier.

[aiba]

@bbatsov @benedekfazekas In the course of implementing this, I realized clj-refactor will also insert test declarations (cljr--add-test-declarations) to the generated ns form. That requires cljr-specific function calls, so I'm stuck on how to keep that functionality in a cider version.

Should we proceed bringing this into cider and drop the add-test-declarations feature, or does this belong in clj-refactor after all? Or is there a 3rd way?

[bbatsov]

This should be in cider as well.

[aiba]

cljr accomplishes cljr--add-test-declarations by parsing/manipulating the ns form with core functions like cljr--insert-in-ns.

For a brand new file, cider could do this with string concatenation. But if users wanted to run cljr--add-test-declarations to a file with an existing namespace form, it would be hard for cider to provide this without cljr.

So I am hesitant to move cljr--add-test-declarations from cljr to cider, going from high-level ns-manipulating functions to string formatting, and also losing the ability to add test declarations to existing namespace forms.

Open to suggestions / different ways of thinking about this.

[bbatsov]

cljr accomplishes cljr--add-test-declarations by parsing/manipulating the ns form with core functions like cljr--insert-in-ns.

Define "parsing". I highly doubt cljr does anything particularly complex there, as I seem to recall this functionality is implemented completely in Elisp.

[Malabarba]

@aiba

Open to suggestions / different ways of thinking about this.

FWIW, I think things like add-test-declaration, add-imports and add-require are fundamental enough that they could be in clojure-mode. The only reason I haven't already copied them is that some of them depend on yasnippet and I wanted to rewrite them with regular minibuffer prompts.

@bbatsov

Define "parsing". I highly doubt cljr does anything particularly complex there, as I seem to recall this functionality is implemented completely in Elisp.

Yes, it's in elisp, but it's still a fair amount of code.
Here's the function definition:

(defun cljr--add-test-declarations ()
  (save-excursion
    (let* ((ns (clojure-find-ns))
           (source-ns (cljr--find-source-ns-of-test-ns ns (buffer-file-name))))
      (cljr--insert-in-ns ":require")
      (when source-ns
        (insert "[" source-ns " :as sut]"))
      (cljr--insert-in-ns ":require")
      (insert (cond
               ((cljr--project-depends-on-p "midje")
                cljr-midje-test-declaration)
               ((cljr--project-depends-on-p "expectations")
                cljr-expectations-test-declaration)
               ((cljr--cljc-file?)
                cljr-cljc-clojure-test-declaration)
               (t cljr-clojure-test-declaration))))
    (indent-region (point-min) (point-max))))

Note how many other cljr- functions it calls. Just as an example, here's another one of those functions:

(defun cljr--insert-in-ns (type &optional cljs?)
  "DOC"
  (cljr--goto-ns)
  (when cljs?
    (cljr--goto-cljs-branch))
  (if (cljr--search-forward-within-sexp (concat "(" type))
      (if (looking-at " *)")
          (progn
            (search-backward "(")
            (forward-list 1)
            (forward-char -1)
            (insert " "))
        (search-backward "(")
        (forward-list 1)
        (forward-char -1)
        (newline-and-indent))
    (forward-list 1)
    (forward-char -1)
    (newline-and-indent)
    (insert "(" type " )")
    (forward-char -1)))

Note that it also calls 3 more cljr- functions.

[aiba]

Yes, pretty much what @Malabarba said. It didn't seem right to have all that code duplicated in both cljr and cider. Moving all of add-test-declaration, add-imports and add-require, to cider makes sense, but is a rather extensive refactor beyond my comfort level right now.

[bbatsov]

I'll have to take a closer look at the code there. @Malabarba's right that in principle such functions belong to clojure-mode, but they might be leveraging some middleware. I don't use cljr and my familiarity with the implementation there is superficial.

[bbatsov]

A simple dirty fix would be to check in cljr for the new function and call it if available, otherwise fallback to the old one. In general, however, I'd prefer us to do more research/work and properly migrate all such functionality to cider/clojure-mode.

[aiba]

I like the simple dirty fix as as short term solution, and agree about properly migrating the functionality for the long term. I'll start with a PR for clj-refactor for the dirty fix.

[Malabarba]

Like I said on the clj-refactor PR. I think this should return (clojure-expected-ns) when not connected, instead of erroring out.

[bbatsov]

If were going to preserve the functionality in clojure-mode (which will be beneficial for inf-clojure users) - this makes sense.

[aiba]

It's your guys' call about whether to preserve the functionality. Do you know if clojure-expected-ns is commonly used from other places besides cljr--add-ns-if-blank-clj-file? Personally I would not miss it if it got removed from clojure-mode.

[bbatsov]

Well, I guess some people are using clj-refactor with inf-clojure, but I doubt there are many of them.