Skip to content

[Fix #310,#311] clojure-expected-ns with src/cljc #312

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

Merged
merged 1 commit into from
Jul 29, 2015

Conversation

expez
Copy link
Member

@expez expez commented Jul 29, 2015

When the source path is src/{clj,cljc,cljs} instead of just src/
clojure-expected ns would create namespaces like clj.my-project.my-ns
whereas what's wanted is my-project.my-ns.

Reading boot.clj or project.clj to find out the user's src dirs is out
of scope for clojure-mode, so we use the simply heuristic that no
namespace should start with clj, cljc or cljs because these are the
idiomatic source directories in multi-source projects.

When improving clojure-expected-ns I extracted out two utilities,
clojure-project-dir and clojure-project-relative-path. These
utilities already exist in clj-refactor so I opted to make them public
rather than private, as they are generally useful.

(- (length (file-name-extension (buffer-file-name) t))))))
(replace-regexp-in-string
"_" "-" (mapconcat 'identity (cdr (split-string relative "/")) "."))))
(defun clojure-project-dir ()
Copy link
Member

Choose a reason for hiding this comment

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

Turns out this implementation is problematic. See clojure-emacs/cider#1215

Copy link
Member Author

Choose a reason for hiding this comment

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

This fix solves the problematic namespaces for 99.9% of affected projects. Let's solve one problem at a time?

Copy link
Member

Choose a reason for hiding this comment

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

I mentioned this mostly because if we have the same function here, there'll be little point in fixing the problem in cider. Anyways, that's not terribly important right now.

@expez expez force-pushed the improve-clojure-expected-ns branch from 6b299d9 to faa9941 Compare July 29, 2015 13:40
(sans-file-sep (mapconcat 'identity (cdr (split-string sans-file-type "/")) "."))
(sans-underscores (replace-regexp-in-string "_" "-" sans-file-sep)))
;; Drop prefix from ns for projects with structure src/{clj,cljs,cljc}
(replace-regexp-in-string "\\`\\(clj.\\|cljs.\\|cljc.\\)" "" sans-underscores)))
Copy link
Member

Choose a reason for hiding this comment

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

clj[sc]?.

Not sure if something isn't needed for cljx as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated with cljx support too

@expez expez force-pushed the improve-clojure-expected-ns branch 2 times, most recently from 0b9d5de to 7dcb52e Compare July 29, 2015 13:53
(sans-file-sep (mapconcat 'identity (cdr (split-string sans-file-type "/")) "."))
(sans-underscores (replace-regexp-in-string "_" "-" sans-file-sep)))
;; Drop prefix from ns for projects with structure src/{clj,cljs,cljc}
(replace-regexp-in-string "\\`clj[scx]?." "" sans-underscores)))
Copy link
Member

Choose a reason for hiding this comment

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

Is . here for any character or this an oversight?

Copy link
Member Author

Choose a reason for hiding this comment

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

The bad ns look like clj.foo.bar.baz if we don't remove the . after clj then that will be the first character in the ns.

Copy link
Member

Choose a reason for hiding this comment

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

That's why I asked - . will match any character. You need to escape it.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@bbatsov
Copy link
Member

bbatsov commented Jul 29, 2015

Mention those 2 bugfixes in the changelog as well.

@expez expez force-pushed the improve-clojure-expected-ns branch from 7dcb52e to 118bbfc Compare July 29, 2015 14:06
…c/cljc

When the source path is src/clj{,c,s,x} instead of just src/
clojure-expected ns would create namespaces like clj.my-project.my-ns
whereas what's wanted is my-project.my-ns.

Reading boot.clj or project.clj to find out the user's src dirs is out
of scope for clojure-mode, so we use the simply heuristic that no
namespace should start with clj, cljc or cljs because these are the
idiomatic source directories in multi-source projects.

When improving clojure-expected-ns I extracted out two utilities,
clojure-project-dir and clojure-project-relative-path.  These
utilities already exist in clj-refactor so I opted to make them public
rather than private, as they are generally useful.
@expez expez force-pushed the improve-clojure-expected-ns branch from 118bbfc to 4be6843 Compare July 29, 2015 14:07
@expez
Copy link
Member Author

expez commented Jul 29, 2015

Mention those 2 bugfixes in the changelog as well.

done

bbatsov added a commit that referenced this pull request Jul 29, 2015
@bbatsov bbatsov merged commit 1fde668 into clojure-emacs:master Jul 29, 2015
@bbatsov
Copy link
Member

bbatsov commented Jul 29, 2015

Everything looks solid now. Thanks!

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

Successfully merging this pull request may close these issues.

2 participants