Add finding docs functionality from ClojureDocs #64

liquidz · 2019-07-10T13:53:16Z

PR for clojure-emacs/cider#2663
Introduce finding docs functionality from not Grimoire but ClojureDocs

The commits are consistent with our contribution guidelines
You've added tests to cover your change(s)
All tests are passing
The new code is not generating reflection warnings

bbatsov · 2019-07-17T06:03:32Z

src/orchard/clojuredocs.clj

@@ -0,0 +1,35 @@
+(ns orchard.clojuredocs
+  "Find docs from ClojureDocs and retrieve the result as a map."
+  {:author "Masashi Iizuka"}


Please, add to the metadata that the namespace was added in version 0.5.

bbatsov · 2019-07-17T06:04:05Z

src/orchard/clojuredocs.clj

+(defn- map-from-key [f coll]
+  (reduce #(assoc %1 (f %2) %2) {} coll))
+
+(defn update-documents-cache!


I guess we can just name this update-cache!, as it's clear from the namespace which cache we are referring to.

bbatsov · 2019-07-17T06:08:06Z

src/orchard/clojuredocs.clj

+         (reset! cache))
+    true))
+
+(defn find-document


I wonder if we can't come up with a better name here, as the word document implies something else. Perhaps find-doc (for documentation). Can't think of a great name, but names are important.

bbatsov · 2019-07-17T06:11:04Z

Apart from my small remarks the changes look good to me. I just wonder if now we should replace the usage of the see-also.edn with this and weather to integrate more of this data (e.g. notes and examples) into the info as well. //cc @arichiardi @SevereOverfl0w @dpsutton

SevereOverfl0w · 2019-07-17T07:13:40Z

It would be nice to carry this along with the info as well, given this is cached it's probably going to have a reasonable performance. We are loading the full 2.8M string into memory with the slurp, so that might be something to consider changing, but I haven't looked deeply at the memory meter and considered whether we care.

liquidz · 2019-07-17T13:44:22Z

@bbatsov Thank you for your reviewing!
I fixed them.

arichiardi · 2019-07-17T13:48:41Z

The patch looks good to me apart from the slurp of the whole thing which we could maybe do lazily ad hoc if we had a different data format (one file per ClojureDocs entry?). Could be an optimization for later.

On another note we should potentially merge this to https://github.com/cljs/api for ClojureScript support.

bbatsov · 2019-07-17T14:27:14Z

src/orchard/clojuredocs.clj

+
+  Return nil if there is no matching document."
+  {:added "0.5.0"}
+  [export-edn-file ns-name var-name]


One final remark - I think here edn-file should be an optional param, as we know the default location of the file after it's downloaded and specifying it explicitly all the time seems redundant to me.

bbatsov · 2019-07-17T14:32:51Z

src/orchard/clojuredocs.clj

+  A EDN format file is expected to the `export-edn-file` argument.
+  (e.g. https://clojuredocs-edn.netlify.com/export.edn)"
+  {:added "0.5.0"}
+  [export-edn-file]


I think we should make this param optional and load the cache straight from the default url.

bbatsov · 2019-07-17T14:35:38Z

@liquidz After reading the code once more I thought of something else - it'd be nice if we were saving the export file locally, so it could be loaded directly on future invocations of Orchard. We can add some load-cache function which checks if the file is locally available (and it's not older than a week) and this function can call update-cache! then. I think that'd be a more user-friendly experience in general.

liquidz · 2019-07-17T14:55:39Z

@bbatsov I thought to do that on the editor plugin side.
But because it adds extra work, I agree to save the exported file locally on orchard side.

How about to save the file in $XDG_CACHE_HOME/orchard/clojuredocs/export.edn ?
cf. https://standards.freedesktop.org/basedir-spec/latest/

bbatsov · 2019-07-17T15:35:58Z

Yeah, that sounds good to me.

SevereOverfl0w · 2019-07-17T19:34:54Z

https://github.com/soc/directories-jvm even if not used, should have it's methods ported, in order to properly support windows & mac properly.

liquidz · 2019-07-17T22:20:33Z

@SevereOverfl0w Thanks!
I'll port some of them!

liquidz · 2019-07-19T17:07:41Z

@bbatsov @SevereOverfl0w Fixed!

I tested orchard.directory/cache-dir for Windows on AppVeyor.
https://ci.appveyor.com/project/liquidz/windows-test/builds/26108911
Tested codes (same as orchard.directory/cache-dir) are here:
https://github.com/liquidz/windows-test/blob/master/src/windows_test/directory.clj

bbatsov · 2019-07-20T06:58:12Z

src/orchard/directory.clj

@@ -0,0 +1,67 @@
+(ns orchard.directory


I think this namespace should be named in some more generic way (provided we want to keep there os-related functionality). Maybe orchard.os or orchard.platform? Not sure how this is typically named.

I like orchard.os shorter!

bbatsov · 2019-07-20T06:58:52Z

src/orchard/misc.clj

@@ -3,8 +3,19 @@
   [clojure.java.io :as io]
   [clojure.string :as str]))

+(def os-name


I think we can put the os-* in the new namespace as well. We'll just keep the existing functions here for compatibility and deprecate them.

bbatsov · 2019-07-20T07:00:24Z

src/orchard/directory.clj

+   :added "0.5.0"}
+  (:require [clojure.java.io :as io]
+            [clojure.string :as str]
+            [orchard.misc :as u])


I think misc is a better alias than u. :-) The weird aliases in the codebase today are due to the fact that this namespace was named util in cider-nrepl (if I recall correctly that is).

liquidz · 2019-07-20T07:55:01Z

@bbatsov Thank you for your reviewing! I fixed them!

bbatsov · 2019-07-20T10:58:00Z

Great work! I'll cut a new beta now, so you can update cider-nrepl accordingly.

bbatsov · 2019-07-20T11:05:36Z

Hmm, seems I pushed beta-9 from my other PC without doing git push and now I don't have access to the other computer. Guess we'll mess up a bit the git history now, but whatever.

bbatsov · 2019-07-20T11:07:03Z

0.5.0-beta10 is out!

liquidz · 2019-07-20T11:13:32Z

Thanks! I'll create a PR to cider-nrepl.

liquidz force-pushed the feature/clojuredocs branch from 007eac0 to 445e785 Compare July 10, 2019 13:55

bbatsov reviewed Jul 17, 2019

View reviewed changes

liquidz force-pushed the feature/clojuredocs branch from 445e785 to 6de9db7 Compare July 17, 2019 13:38

bbatsov reviewed Jul 17, 2019

View reviewed changes

liquidz force-pushed the feature/clojuredocs branch 2 times, most recently from 01edbb5 to 571f9bd Compare July 19, 2019 16:55

bbatsov reviewed Jul 20, 2019

View reviewed changes

liquidz added 2 commits July 20, 2019 16:44

Port cache-dir from soc/directories-jvm

c486384

Add finding docs functionality from ClojureDocs

e707c3a

liquidz force-pushed the feature/clojuredocs branch from 571f9bd to e707c3a Compare July 20, 2019 07:49

bbatsov merged commit e664c92 into clojure-emacs:master Jul 20, 2019

liquidz mentioned this pull request Jul 23, 2019

Add a middleware to find docs from ClojureDocs clojure-emacs/cider-nrepl#628

Merged

5 tasks

SevereOverfl0w mentioned this pull request Aug 9, 2019

fix replantexample SevereOverfl0w/vim-replant#21

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add finding docs functionality from ClojureDocs #64

Add finding docs functionality from ClojureDocs #64

liquidz commented Jul 10, 2019

bbatsov Jul 17, 2019

bbatsov Jul 17, 2019

bbatsov Jul 17, 2019

bbatsov commented Jul 17, 2019

SevereOverfl0w commented Jul 17, 2019

liquidz commented Jul 17, 2019

arichiardi commented Jul 17, 2019

bbatsov Jul 17, 2019

bbatsov Jul 17, 2019

bbatsov commented Jul 17, 2019

liquidz commented Jul 17, 2019

bbatsov commented Jul 17, 2019

SevereOverfl0w commented Jul 17, 2019

liquidz commented Jul 17, 2019

liquidz commented Jul 19, 2019

bbatsov Jul 20, 2019

liquidz Jul 20, 2019

bbatsov Jul 20, 2019

bbatsov Jul 20, 2019

liquidz commented Jul 20, 2019

bbatsov commented Jul 20, 2019

bbatsov commented Jul 20, 2019

bbatsov commented Jul 20, 2019

liquidz commented Jul 20, 2019

Add finding docs functionality from ClojureDocs #64

Add finding docs functionality from ClojureDocs #64

Conversation

liquidz commented Jul 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Jul 17, 2019

SevereOverfl0w commented Jul 17, 2019

liquidz commented Jul 17, 2019

arichiardi commented Jul 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Jul 17, 2019

liquidz commented Jul 17, 2019

bbatsov commented Jul 17, 2019

SevereOverfl0w commented Jul 17, 2019

liquidz commented Jul 17, 2019

liquidz commented Jul 19, 2019

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

liquidz commented Jul 20, 2019

bbatsov commented Jul 20, 2019

bbatsov commented Jul 20, 2019

bbatsov commented Jul 20, 2019

liquidz commented Jul 20, 2019