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

clojure-expected-ns and custom :source-paths #372

Closed
aiba opened this issue Mar 20, 2016 · 8 comments
Closed

clojure-expected-ns and custom :source-paths #372

aiba opened this issue Mar 20, 2016 · 8 comments

Comments

@aiba
Copy link

aiba commented Mar 20, 2016

(clojure-expected-ns) returns the wrong ns if you organize project.clj with :source-paths ["src/server" "src/client" "src/common"]. clojure-expected-ns just chops off the "src/" from the path and makes a ns out of the rest, so it adds an extra e.g. "server." prefix.

It already special-cases "src/clj" and "src/cljs" via a regexp. What if we just made that regexp a defcustom for people who organize their project with custom source-paths?

If that's the right fix, I'd be happy to take a stab at a PR. If there's a better way, I'd love to hear it!

@aiba
Copy link
Author

aiba commented Mar 20, 2016

If we had the classpath from cider, we could just use it directly rather than resorting to filesystem hacks. The following function seems to work (but depends on (cider-sync-request:classpath)).

(defun expected-ns-from-classpath ()
  (let* ((file-path (buffer-file-name))
         (rp (reduce
              (lambda (r p)
                (let ((rp (when (string/starts-with file-path p)
                            (substring file-path (length p)))))
                  (if (and r rp (< (length rp) (length r)))
                      rp
                    (or rp r))))
              (cider-sync-request:classpath)
              :initial-value nil)))
    (when rp
      (let* ((ns (substring rp 1 (- (length (file-name-extension rp t)))))
             (ns (replace-regexp-in-string "/" "." ns))
             (ns (replace-regexp-in-string "_" "-" ns)))
        ns))))

Perhaps this function should be added to cider rather than adding more hacks to clojure-expected-ns?

@bbatsov
Copy link
Member

bbatsov commented Mar 20, 2016

It already special-cases "src/clj" and "src/cljs" via a regexp. What if we just made that regexp a defcustom for people who organize their project with custom source-paths?

That's a reasonable approach.

Perhaps this function should be added to cider rather than adding more hacks to clojure-expected-ns?

I'm guessing this is some pseudocode, right? :-) I'll have to dwell on it a bit, as it's not immediately apparent to me what this is supposed to do. In general, I'm not opposed to adding such functionality to cider, just keep in mind that not everyone is using cider - some people use inf-clojure, some use monroe, etc. Maybe it's worth implementing both ideas, maybe just the second one.

@aiba
Copy link
Author

aiba commented Mar 20, 2016

It's actually working code, sorry it's not clear, I'm not as fluent in elisp as clojure. It first finds the relative path (rp) of the buffer, relative to some entry in the classpath. The reduce also finds the shortest such relative path (in case "src/" and "src/foo/" are both in the classpath). Then it just drops the extension, replaces "/" with "." and "_" with "-", and voila, a namespace.

I'm thinking this might make a nice addition to cider. Then tools could use cider (if available) to find the expected namespace. The main use case I have in mind is cljr-add-ns-to-blank-clj-files, which can pretty safely assume cider is available.

@aiba
Copy link
Author

aiba commented Mar 20, 2016

This version of the function may be a little more clear:

(defun expected-ns ()
  (let* ((file-path (buffer-file-name))
         (relpath (->> (cider-sync-request:classpath)
                       (-map (lambda (p)
                              (when (string/starts-with file-path p)
                                (substring file-path (length p)))))
                       (-filter 'identity)
                       (-sort #'string<)
                       (car))))
    (when relpath
      (->> (substring relpath 1) ; remove leading /
           (file-name-sans-extension)
           (replace-regexp-in-string "/" ".")
           (replace-regexp-in-string "_" "-")))))

@bbatsov
Copy link
Member

bbatsov commented Mar 20, 2016

Sounds good. Just a few things in mind:

  • for cider you'll have to use seq.el instead of dash.el (they are pretty similar)
  • you'll also need to add a few unit tests (just stub the classpath request)
  • we should probably move the add-ns functionality to cider (@benedekfazekas proposed this himself on slack)

With this in mind - feel free to create a cider PR.

@aiba
Copy link
Author

aiba commented Mar 20, 2016

Sounds good. I'll start with a PR to get the function into cider. Then we can change the cljr dependency and move other things around separately.

@benedekfazekas
Copy link
Member

coolio!

@aiba
Copy link
Author

aiba commented Mar 24, 2016

See also clojure-emacs/cider#1622

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

No branches or pull requests

3 participants