Ability to specify context in `cljr-magic-require-namespaces`

[vemv]

cljr-magic-require-namespaces doesn't currently allow specifying its intended context (:clj, :cljs).

This can easily cause incorrect suggestions for cljr-slash.

I'd suggest one of the following:

• Introduce new defcustom, e.g. cljr-jvm-magic-require-namespaces
  • This would be a list like '("clojure.java.io"), specifying which values of cljr-magic-require-namespaces are intended for JVM clojure.
  • Same for cljs
• (Preferred) Allow cljr-magic-require-namespaces to also include lists
  • Example difference:

(defcustom cljr-magic-require-namespaces
  '(("set"  . "clojure.set")
-   ("io"   . "clojure.java.io")
+   ("io"   "clojure.java.io" "clj")
+   ("io"   "foo.io" "cljs")
    ("str"  . "clojure.string")
    ("walk" . "clojure.walk")
    ("zip"  . "clojure.zip")))

i.e. we could mix and match cons cells (("set" . "clojure.set")) with lists having the extra property ("io" "clojure.java.io" "clj").

Both approaches are intended to be backwards-compatible.

Notes on semantics

• There are two values that must be expressible: clj, cljs
  • cljc would be a no-op (since by default, entries already are context-agnostic)
• A value such as "io" "clojure.java.io" "clj" is intended to mean "please only possibly suggest clojure.java.io if the filename is .clj"
  • It does not, however, mean "clojure.java.io is the only acceptable completion if the filename is .clj"
  • refactor-nrepl is ultimately responsible for deciding what choice(s) will be offered.

Final decision

#530 (comment)

[dgtized]

I'm curious why the defcustom should still be an alist prefixed by the namespace refer. If the intention is to send the entire defcustom to the middleware, can't we parse the recommended as target in the middleware? I wonder if we could make it a list of the recommendation string with a list of valid language contexts, where a nil list indicates all language contexts are allowed.

(defcustom cljr-magic-require-namespaces
  '(("clojure.set :as set")
    ("clojure.java.io :as io" ("clj"))
    ("foo.io :as io" ("cljs"))
    ("clojure.string :as str")
    ("clojure.walk :as walk")
    ("clojure.zip :as zip")))

In the future this could support examples where the recommended namespace contained a refers or a reader conditional. Ie an example like: '("clojure.set :refer [union] :as set"). I could also imagine prefixing the restrictions with a keyword like :include/:exclude, or :only/:except to account for the other language contexts possibilities. Ie something like: ("clojure.java.io :as io" :only ("clj")) or ("foo.io :as io" :except ("clj")).

As a reminder, while I think the majority of users are only encountering this with cljc, cljs, and clj, the official docs for reader conditionals includes cljr, and default, and while I haven't encountered it in the wild, Babashka does supports :bb. Even if those are not yet supported by this functionality, I think it makes sense to assume future compatibility will be desired.

This would not be quite as compatible with the existing format, but I think it allows for more flexibility moving forward.

[vemv]

In the future this could support examples where the recommended namespace contained a refers or a reader conditional.

While desiring such a thing makes sense, I'd like to favor a different approach: real project usage (i.e. whatever you have in your namespace files) is the ultimate indication to refactor-nrepl of what is intended.

This is more concise (you have to declare very few, basic preferences), and more flexible (goes on a per-project basis; their different customs don't get mixed up). We also don't require a migration path for users (which can be hard to communicate IME).

It also goes well with the CIDER's overall philosophy of gathering insights at runtime, instead of requiring extensive, upfront, static configuration.

I'm not definitely closing the door to this feature, but in the end, making the middleware accept more inputs (e.g. a new defcustom, while the old one remains acceptable) is not a breaking change.

Thanks!

[dgtized]

I think on further reflection, the major concern I have about this, despite it being backwards compatible is that it feels like a very unintuitive syntax to encounter in Emacs without any context. I'm struggling to think of any other API I've seen using a syntax like this? It's not discoverable without specifically reading the docs for the defcustom (which I'm not sure we can fix), but more importantly if I saw it in the wild I don't know as I would intuitively understand that it was limiting it to those contexts.

What if we use keyword like :only to help indicate intent:

(defcustom cljr-magic-require-namespaces
  '(("set"  . "clojure.set")
   ("io"   "clojure.java.io" :only ("clj"))
   ("io"   "foo.io" :only ("cljs"))
    ("str"  . "clojure.string")
    ("walk" . "clojure.walk")
    ("zip"  . "clojure.zip")))

I believe that is still backwards compatible, but I think that might be easier to follow at first glance? It would also support adding :exclude at some future point, while still maintaining compatibility. Let me know what you think.

[vemv]

Sounds good, let's go for it 👍