Skip to content

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

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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!

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

Successfully merging this pull request may close these issues.

2 participants