[Fix #451] respect project root dir suggested by custom function #452

mbuczko · 2017-10-06T16:23:34Z

This is a humble attempt to get clojure-mode respecting project root directory suggested by a custom function (like projectile-project-root), as described in #451

Malabarba · 2017-10-07T01:42:01Z

clojure-mode.el

-    (when (> (length choices) 0)
-      (car (sort choices #'file-in-directory-p)))))
+    (or (and (fboundp clojure-project-root-locating-function)
+             (condition-case err


This is just ignore-errors

Malabarba · 2017-10-07T01:43:59Z

clojure-mode.el

@@ -192,6 +192,11 @@ Out-of-the box `clojure-mode' understands lein, boot and gradle."
          (and (listp value)
               (cl-every 'stringp value))))

+(defcustom clojure-project-root-locating-function nil
+  "Alternative function to locate clojure project root directory, eg. projectile-project-root"
+  :type 'symbol


I think there's a type: 'function

Malabarba · 2017-10-07T01:44:54Z

clojure-mode.el

+(defcustom clojure-project-root-locating-function nil
+  "Alternative function to locate clojure project root directory, eg. projectile-project-root"
+  :type 'symbol
+  :safe 'symbolp)


This variable is funcalled, so no value is safe for it. In fact, we should probably mark it as risky.

bbatsov · 2017-10-07T05:17:01Z

clojure-mode.el

@@ -192,6 +192,11 @@ Out-of-the box `clojure-mode' understands lein, boot and gradle."
          (and (listp value)
               (cl-every 'stringp value))))

+(defcustom clojure-project-root-locating-function nil


I'd name this just clojure-project-root-function.

bbatsov · 2017-10-07T05:17:20Z

clojure-mode.el

@@ -192,6 +192,11 @@ Out-of-the box `clojure-mode' understands lein, boot and gradle."
          (and (listp value)
               (cl-every 'stringp value))))

+(defcustom clojure-project-root-locating-function nil
+  "Alternative function to locate clojure project root directory, eg. projectile-project-root"


Why alternative - let's just set it by default to the currently existing function.

bbatsov · 2017-10-07T05:18:11Z

clojure-mode.el

@@ -1607,8 +1612,12 @@ Return nil if not inside a project."
                        (mapcar (lambda (fname)
                                  (locate-dominating-file dir-name fname))
                                clojure-build-tool-files))))
-    (when (> (length choices) 0)
-      (car (sort choices #'file-in-directory-p)))))
+    (or (and (fboundp clojure-project-root-locating-function)


I'd simply remove those check here - if the user set something explicitly we can assume they know what they are doing. Fallbacks are pointless and confusing here.

bbatsov · 2017-10-07T05:20:04Z

clojure-mode.el

@@ -192,6 +192,11 @@ Out-of-the box `clojure-mode' understands lein, boot and gradle."
          (and (listp value)
               (cl-every 'stringp value))))

+(defcustom clojure-project-root-locating-function nil


The defcustom should also have a :package-version property pointing this was added in version 5.7.0.

vspinu · 2017-10-07T15:24:58Z

Shouldn't this be using project-find-functions (new emacs 25 functionality in project.el)? If users can already add hooks to project-find-functions, then this functionality seems redundant.

bbatsov · 2017-10-07T15:35:22Z

Shouldn't this be using project-find-functions (new emacs 25 functionality in project.el)? If users can already add hooks to project-find-functions, then this functionality seems redundant.

In theory - yes. In practice this is more or less a prototype that wasn't really polished. I was actually discussing this with its author just a couple of days ago. He wanted us to collaborate on providing more extension points for Projectile, but frankly I don't really see much point in working towards this universal API given the fact I'm not really interested in using it and apart from a few generic functions each project management library has way too many specifics to it. On the other side - this could allow a lot of modes to drop at least the product root detection code they cart along.

Type changed to function and marked as risky.

mbuczko · 2017-10-07T18:47:22Z

@bbatsov I applied your suggestions but to get rid of unnecessary fallbacks and set clojure-project-root-function customization by default to the function currently used to calculate project root path I had to split clojure-project-dir into 2 parts. I'm not sure if that's done right, any comments nicely welcomed :)

bbatsov · 2017-10-07T18:51:05Z

Why did you split it in two? I don't see any reason to do, even after taking a look at the code.

mbuczko · 2017-10-07T19:14:06Z

This was to set clojure-project-root-function to default value (function) which would call what we have right now in clojure-mode to calculate root-path. I couldn't set it up simply to clojure-project-dir as having my funcall inside, this function would be called in recursive loop (funcall would call clojure-project-dir again which in turn would call funcall and so on...).

bbatsov · 2017-10-07T20:34:20Z

Ah, yeah - the name you have to the old function confused me a bit. This looks ok overall.

Malabarba · 2017-10-07T22:54:27Z

clojure-mode.el

@@ -192,6 +192,12 @@ Out-of-the box `clojure-mode' understands lein, boot and gradle."
          (and (listp value)
               (cl-every 'stringp value))))

+(defcustom clojure-project-root-function 'clojure-project-root-path


Change the ' to a #'

Malabarba · 2017-10-07T22:54:57Z

Besides that last comment, it looks good to me.

bbatsov · 2017-10-08T05:16:38Z

It still needs a changelog entry.

clojure-project-root-function defaults to #'clojure-project-root-path now.

mbuczko · 2017-10-08T09:56:08Z

Changelog updated.

…ectory customizable (clojure-emacs#452)

dgutov · 2017-11-02T13:01:23Z

this could allow a lot of modes to drop at least the product root detection code they cart along

It would allow e.g. M-x project-find-regexp to use Projectile. And some other functions in core Emacs.

mbuczko added 2 commits October 6, 2017 17:19

[Fix #451] respect project root dir suggested by projectile

beed375

Added customizable function to locate project root directory

5ffb46c

mbuczko changed the title ~~[Fix #451] respect project root dir suggested by projectile~~ [Fix #451] respect project root dir suggested by custom function Oct 6, 2017

Malabarba reviewed Oct 7, 2017

View reviewed changes

bbatsov reviewed Oct 7, 2017

View reviewed changes

mbuczko added 2 commits October 7, 2017 17:53

Customization renamed to clojure-project-root-function

3e58acd

Type changed to function and marked as risky.

clojure-project-dir delegates call to clojure-project-root-function

766fc5b

Got rid of unecessary funcall

cd71981

Malabarba reviewed Oct 7, 2017

View reviewed changes

Changelog updated.

607a2ce

clojure-project-root-function defaults to #'clojure-project-root-path now.

bbatsov merged commit 35f5d71 into clojure-emacs:master Oct 8, 2017

slipset pushed a commit to slipset/clojure-mode that referenced this pull request Nov 1, 2017

[Fix clojure-emacs#451] Make the function determining the project dir…

ced3af1

…ectory customizable (clojure-emacs#452)

vspinu mentioned this pull request Nov 2, 2017

default directory changed? emacs-ess/ESS#461

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix #451] respect project root dir suggested by custom function #452

[Fix #451] respect project root dir suggested by custom function #452

mbuczko commented Oct 6, 2017 •

edited

Loading

Malabarba Oct 7, 2017

Malabarba Oct 7, 2017

Malabarba Oct 7, 2017

bbatsov Oct 7, 2017

bbatsov Oct 7, 2017

bbatsov Oct 7, 2017

bbatsov Oct 7, 2017

vspinu commented Oct 7, 2017

bbatsov commented Oct 7, 2017

mbuczko commented Oct 7, 2017

bbatsov commented Oct 7, 2017

mbuczko commented Oct 7, 2017

bbatsov commented Oct 7, 2017

Malabarba Oct 7, 2017

Malabarba commented Oct 7, 2017

bbatsov commented Oct 8, 2017

mbuczko commented Oct 8, 2017

dgutov commented Nov 2, 2017

[Fix #451] respect project root dir suggested by custom function #452

[Fix #451] respect project root dir suggested by custom function #452

Conversation

mbuczko commented Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vspinu commented Oct 7, 2017

bbatsov commented Oct 7, 2017

mbuczko commented Oct 7, 2017

bbatsov commented Oct 7, 2017

mbuczko commented Oct 7, 2017

bbatsov commented Oct 7, 2017

Choose a reason for hiding this comment

Malabarba commented Oct 7, 2017

bbatsov commented Oct 8, 2017

mbuczko commented Oct 8, 2017

dgutov commented Nov 2, 2017

mbuczko commented Oct 6, 2017 •

edited

Loading