Also consider cljc files for namespaces-on-classpath #91

eval · 2023-06-15T12:08:21Z

Fixes #90

vemv

Nice find!

Please bear in mind that there's https://github.com/clojure-emacs/clj-suitable for cljs completions.

Because namespaces-on-classpath only returns the namespace name, it's safe (not dependent on reader conditionals) to return results for clj, cljc, and cljs alike.

It would seem desirable to craft a test for namespaces-on-classpath ensuring that all 3 kinds of results are returned. I could contribute a commit if you happen to be in a rush.

Cheers - V

eval · 2023-06-15T13:12:12Z

Because namespaces-on-classpath only returns the namespace name, it's safe (not dependent on reader conditionals) to return results for clj, cljc, and cljs alike.

I quickly scanned clj-suitable. Do I understand your suggestion correctly in that we'd add (.endsWith file ".cljs") to namespaces-on-classpath, label the candidates in some way and then adjust compliment.sources.namespaces-and-classes to only filter out the clj(c) namespaces based on that labeling?

It would seem desirable to craft a test for namespaces-on-classpath ensuring that all 3 kinds of results are returned. I could contribute a commit if you happen to be in a rush.

🙏🏻

vemv · 2023-06-15T13:30:14Z

Currently, the following is returned:

{:candidate ns-str, :type :namespace}

...it would seem easy enough to make it also include :extension ".cljc"? Depending on the actual filename.

(Note that I chose the word :extension, not :context, :platform, etc. This makes the notion very easy to define and does not force us to think of other semantics for now. Which can be added later)

Then, consumers like clj-suitable could remove entries where the :type was :namespace and the :extension was ".clj" i.e. not .cljs or .cljc.

eval · 2023-06-15T15:41:34Z

Adjusted to allow for getting cljs namespaces out as well. Fully bw-compatible.
Let me know WYT!

src/compliment/utils.clj

vemv

LGTM as far as I'm concerned

vemv · 2023-06-15T15:49:44Z

src/compliment/sources/namespaces_and_classes.clj

@@ -93,11 +93,12 @@
         (get-all-full-names prefix))
       ;; If prefix doesn't contain a period, using fuziness produces too many
       ;; irrelevant candidates.
-       (for [^String ns-str (utils/namespaces-on-classpath)
+       (for [{^String ns-str :ns, ^String ext :extension}
+             (utils/namespaces-on-classpath {:name-only false, :extensions #{"clj" "cljc"}})


Possibly, in a future, the value of :extensions could be derived from the surrounding context. (See also)

e.g. if I'm a clj-suitable user, I might be able to convey :context :cljs somehow here, and then :extensions #{"clj" "cljc"}} as seen here would change, dynamically.

Just commenting that as something I'd intend to do while maintaining clj-suitable. Nothing in the PR is preventing that from happening - great work!

eval · 2023-06-15T20:41:35Z

@vemv added tests as that turned out easier than expected. Hope that didn't cross any work you already did.

alexander-yakushev · 2023-06-15T21:09:12Z

Thank you for the PR! Since the scope of the fix has grown, let me grump and bikeshed a little bit.

Maybe this is irrational, but computing and attaching this :extension key to the results rubs me the wrong way. Even though a file extension is a clear-cut concept, I feel like it's a wrong entity to introduce on the Compliment side. I think it is better to attach the whole :file to go together with completions for namespaces. This in theory gives more choice to upstream users of Compliment (however few there are) to filter namespace completions based on the extension but also anything else (file on disk vs inside JAR? I don't know).

I'm also not a big fan of :name-only false approach. I've done exactly the same for candidates before with :tag-candidates true, and it just doesn't feel very clean. Since namespaces-on-classpath is an internal function (but still public), it's better to introduce a separate function, e.g. namespaces+files-on-classpath, with the new functionality. The old function can be deprecated and then removed sometime in the future.

Let me know if any of this sounds dumb.

eval · 2023-06-16T08:36:56Z

I think it is better to attach the whole :file to go together with completions for namespaces.

Makes sense!

it's better to introduce a separate function, e.g. namespaces+files-on-classpath, with the new functionality.

👍🏻 Should we keep the extensions-option in this new function to provide filtering for callers?

alexander-yakushev · 2023-06-16T11:49:50Z

Should we keep the extensions-option in this new function to provide filtering for callers?

I'd rather keep it at the callsite, to be honest. Especially since the new function doesn't return extensions anymore, it shouldn't look at them either. Just return the files and let the caller filter the list.

alexander-yakushev · 2023-06-16T11:52:04Z

src/compliment/utils.clj

+           (for [^String file (all-files-on-classpath classpath)
+                 :when        (and (or (.endsWith file ".clj")
+                                       (.endsWith file ".cljc")
+                                       (.endsWith file ".cljs"))


At this point, (re-find #"\.clj[cs]?$" file) would be better.

alexander-yakushev · 2023-06-16T11:54:27Z

src/compliment/utils.clj

+                                       (.endsWith file ".cljs"))
+                                   (not (.startsWith file "META-INF")))
+                 :let         [file (ensure-no-leading-slash file)
+                               [_ ^String nsname ^String extension] (re-matches #"[^\w]?(.+)\.(clj[sc]?)" file)]


Can actually move this :let up and then use the fact that nsname matches as the filter.

alexander-yakushev · 2023-06-16T11:54:58Z

src/compliment/utils.clj

+                               [_ ^String nsname ^String extension] (re-matches #"[^\w]?(.+)\.(clj[sc]?)" file)]
+                 :when        nsname]
+             (let [ns-str (.. nsname (replace resource-separator ".") (replace "_" "-"))]
+               {:ns ns-str, :file file, :extension extension})))


I second @vemv, let's name the key :ns-str.

alexander-yakushev · 2023-06-16T11:57:23Z

src/compliment/utils.clj

+        (transduce
+         (map :ns)
+         conj
+         (namespaces&files-on-classpath {:extensions #{"clj" "cljc"}}))))


You don't need both into and transduce. It's either (into #{} (map :ns) coll) or (transduce (map :ns) conj #{} coll).

alexander-yakushev · 2023-06-16T11:58:07Z

src/compliment/utils.clj

@@ -208,9 +232,9 @@ Note that should always have the same value, regardless of OS."
    (cache-last-result ::project-resources classpath
      (for [path classpath
            ^String file (list-files path false)
-            :when (not (or (empty? file) (.endsWith file ".clj")
+            :when (not (or (empty? file)
+                           (.endsWith file ".clj") (.endsWith file ".cljc") (.endsWith file ".cljs")
                           (.endsWith file ".jar") (.endsWith file ".class")))]


Also makes sense to pack it into a single regex at this point.

alexander-yakushev · 2023-06-16T11:59:04Z

test/dummy.cljs

@@ -0,0 +1,2 @@
+;; For testing (compliment.t-utils) purpose


Better mention that it is for testing namespaces&files-on-classpath directly.

eval · 2023-06-16T14:43:32Z

@alexander-yakushev thanks for the comments! Changes applied.

vemv · 2023-06-16T15:49:47Z

fwiw, LGTM!

alexander-yakushev

A few nitpicks before we are good to merge.

alexander-yakushev · 2023-06-16T17:43:41Z

src/compliment/sources/namespaces_and_classes.clj

-                           (nscl-matches? prefix ns-str)
-                           (.startsWith ns-str prefix))]
-               {:candidate ns-str, :type :namespace})
+             (for [{^String ns-str :ns-str, ^String file :file} (utils/namespaces&files-on-classpath)


Keys match names now, {:keys [^String ns-str, ^String file]} is preferred.

alexander-yakushev · 2023-06-16T17:44:06Z

src/compliment/sources/namespaces_and_classes.clj

-                           (.startsWith ns-str prefix))]
-               {:candidate ns-str, :type :namespace})
+             (for [{^String ns-str :ns-str, ^String file :file} (utils/namespaces&files-on-classpath)
+                   :when (and


Unnecessary line break after and.

alexander-yakushev · 2023-06-16T17:47:33Z

src/compliment/utils.clj

-                 :when nsname]
-             (.. nsname (replace resource-separator ".") (replace "_" "-")))))))
+    (set (cache-last-result ::namespaces-on-classpath classpath
+           (for [^String file (all-files-on-classpath classpath)


file doesn't need a type hint anymore. Besides, none of the code around used binding pairs alignment, so I'd rather keep it that way.

alexander-yakushev · 2023-06-16T17:48:38Z

src/compliment/utils.clj


 (defn project-resources
  "Returns a list of all non-code files in the current project."
  []
  (let [classpath (classpath)]
    (cache-last-result ::project-resources classpath
-      (for [path classpath
+      (for [path         classpath


alexander-yakushev · 2023-06-16T17:48:57Z

src/compliment/utils.clj

-            :when (not (or (empty? file) (.endsWith file ".clj")
-                           (.endsWith file ".jar") (.endsWith file ".class")))]
+            :when        (not (or (empty? file)
+                                  (re-find #"\.(clj[cs]?|jar|class)" file)))]


Missing $ at the end of the regex.

alexander-yakushev · 2023-06-16T17:54:03Z

src/compliment/utils.clj

-                 :let [[_ ^String nsname] (re-matches #"[^\w]?(.+)\.clj" file)]
-                 :when nsname]
-             (.. nsname (replace resource-separator ".") (replace "_" "-")))))))
+    (set (cache-last-result ::namespaces-on-classpath classpath


set moved out of the caching form which is bad, we want to cache that transformation too. However, in the new function, set doesn't achieve what it used to do before for strings, since it is unlikely that we'll have duplicate filenames (but there could be duplicate namespaces). So, to match the prior behavior, we need something like distinct-by which does not exist. Or might just leave it as is for now (by removing set) and maybe place a ;; TODO for future deduplication.

Good point - added the TODO, but also wondering if it's relevant now that we return clj, cljc and cljs results?

I think this is quite minor, to be honest. There is a slim chance that two same namespaces on the classpath come from different files, and somebody using this new function will show both of them (but not Compliment, since it performs its own deduplication later). And that is... not a big issue:).

...and introduce namespaces&files-on-classpath that yields collection of maps with e.g. :file. Fixes alexander-yakushev#90

alexander-yakushev · 2023-06-17T11:38:56Z

Looks perfect now, thank you for following through!

eval force-pushed the issue-90 branch from 97a32f4 to b093f6a Compare June 15, 2023 12:09

eval marked this pull request as ready for review June 15, 2023 12:09

eval mentioned this pull request Jun 15, 2023

ns completions for cljc-files #90

Closed

vemv reviewed Jun 15, 2023

View reviewed changes

eval force-pushed the issue-90 branch from b093f6a to 63d9bea Compare June 15, 2023 15:40

eval commented Jun 15, 2023

View reviewed changes

src/compliment/utils.clj Outdated Show resolved Hide resolved

vemv approved these changes Jun 15, 2023

View reviewed changes

eval force-pushed the issue-90 branch from 63d9bea to 74c4f1e Compare June 15, 2023 20:39

alexander-yakushev added the enhancement label Jun 15, 2023

eval force-pushed the issue-90 branch 3 times, most recently from a6c4892 to 587dd71 Compare June 16, 2023 09:56

alexander-yakushev reviewed Jun 16, 2023

View reviewed changes

eval force-pushed the issue-90 branch from 587dd71 to f277568 Compare June 16, 2023 14:43

alexander-yakushev reviewed Jun 16, 2023

View reviewed changes

eval force-pushed the issue-90 branch 2 times, most recently from e5d31fa to f4cab7a Compare June 17, 2023 10:54

namespaces-on-classpath yields also cljc candidates

2b02083

...and introduce namespaces&files-on-classpath that yields collection of maps with e.g. :file. Fixes alexander-yakushev#90

eval force-pushed the issue-90 branch from f4cab7a to 2b02083 Compare June 17, 2023 10:58

alexander-yakushev merged commit 55d992c into alexander-yakushev:master Jun 17, 2023

vemv mentioned this pull request Jun 17, 2023

feat: Add ClojureScript support #89

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Also consider cljc files for namespaces-on-classpath #91

Also consider cljc files for namespaces-on-classpath #91

eval commented Jun 15, 2023

vemv left a comment

eval commented Jun 15, 2023

vemv commented Jun 15, 2023 •

edited

Loading

eval commented Jun 15, 2023

vemv left a comment

vemv Jun 15, 2023

eval commented Jun 15, 2023

alexander-yakushev commented Jun 15, 2023 •

edited

Loading

eval commented Jun 16, 2023

alexander-yakushev commented Jun 16, 2023

alexander-yakushev Jun 16, 2023

alexander-yakushev Jun 16, 2023

alexander-yakushev Jun 16, 2023

alexander-yakushev Jun 16, 2023

alexander-yakushev Jun 16, 2023

alexander-yakushev Jun 16, 2023

eval commented Jun 16, 2023

vemv commented Jun 16, 2023

alexander-yakushev left a comment

alexander-yakushev Jun 16, 2023

alexander-yakushev Jun 16, 2023

alexander-yakushev Jun 16, 2023

alexander-yakushev Jun 16, 2023

alexander-yakushev Jun 16, 2023

alexander-yakushev Jun 16, 2023

eval Jun 17, 2023

alexander-yakushev Jun 17, 2023

alexander-yakushev commented Jun 17, 2023

Also consider cljc files for namespaces-on-classpath #91

Also consider cljc files for namespaces-on-classpath #91

Conversation

eval commented Jun 15, 2023

vemv left a comment

Choose a reason for hiding this comment

eval commented Jun 15, 2023

vemv commented Jun 15, 2023 • edited Loading

eval commented Jun 15, 2023

vemv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eval commented Jun 15, 2023

alexander-yakushev commented Jun 15, 2023 • edited Loading

eval commented Jun 16, 2023

alexander-yakushev commented Jun 16, 2023

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

eval commented Jun 16, 2023

vemv commented Jun 16, 2023

alexander-yakushev left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-yakushev commented Jun 17, 2023

vemv commented Jun 15, 2023 •

edited

Loading

alexander-yakushev commented Jun 15, 2023 •

edited

Loading