Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

cljr-slash using suggest-libspecs middleware op #532

Merged
merged 14 commits into from
Oct 11, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 80 additions & 11 deletions clj-refactor.el
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,13 @@ Any other non-nil value means to add the form without asking."
(const :tag "prompt" :prompt)
(const :tag "false" nil)))

(defcustom cljr-slash-uses-suggest-libspec nil
"If t, `cljr-slash' magic require functionality will use the
dgtized marked this conversation as resolved.
Show resolved Hide resolved
`cljr-suggest-libspec' middleware op instead of the
`namespace-aliases' op."
:type 'boolean
:safe #'booleanp)

(defcustom cljr-magic-require-namespaces
'(("io" . "clojure.java.io")
("set" . "clojure.set")
Expand Down Expand Up @@ -1957,6 +1964,61 @@ following this convention: https://stuartsierra.com/2015/05/10/clojure-namespace
(cljr--call-middleware-sync "namespace-aliases")
parseedn-read-str))

(defun cljr--call-middleware-suggest-libspec (alias-ref language-context)
"Suggest libspec entries for an `alias-ref' in a `language-context'.
vemv marked this conversation as resolved.
Show resolved Hide resolved

Returns a candidate list of libspec entry strings. `alias-ref' is
a string representing a namespace alias. `language-context' is a
2 element list representing the language context of the buffer,
followed by the language context at the current point. Assume
same context as buffer if context at current point is nil.

Passes through the custom `cljr-magic-require-namespaces' so that
users can specify default recommended alias prefixes that may not
appear in the project yet."
(seq-let (buffer-context point-context) language-context
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seq-let is probably nice but this lib (and I, coincidentally) only use seq.el for "seq stuff", not for other conveniences which would increase cognitive load to casual maintainers.

I'm generally happy with let* (and prefer let* over let, no matter what)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've addressed this to your recommendation. From my perspective, destructuring binding is much easier to follow over manually parsing out car/cadr of each element, but to each their own. I appreciate your concern about cognitive load from unfamiliar constructs, but it's also frustrating and discouraging as a contributor to so frequently encounter code included in emacs core that is then dismissed at review. Do you have a style guide somewhere that I could follow to discover which are recommended and which are discouraged instead of discovering all of these at review 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 could follow to discover which are recommended and which are discouraged instead of discovering all of these at review time?

Not that I know of

Generally, afaict in clojure-emacs .el code we lean toward simple constructs. Even then, different reviewers might have different preferences.

My genuine recommendation would be to simply be ready to tweak some things upon request. I myself cannot predict the feedback I'll receive in the variety of repos I contibute to - some little churn on superficial things is just part of life! :)

(thread-first
"cljr-suggest-libspecs"
cljr--ensure-op-supported
(cljr--create-msg "lib-prefix" alias-ref
"language-context" buffer-context
"buffer-language-context" buffer-context
"input-language-context" (or point-context buffer-context)
"preferred-aliases" (prin1-to-string cljr-magic-require-namespaces))
(cljr--call-middleware-sync "suggestions")
parseedn-read-str
(seq-into 'list))))

(defun cljr--language-context-at-point ()
"Detects the current language-context at a given point.

Returns a tuple, the first value represents the language context
of the file, the second represents the language context at the
current point if it is within a reader conditional. If either
value is unknown, return nil."
dgtized marked this conversation as resolved.
Show resolved Hide resolved
(list (cond ((cljr--cljc-file-p)
"cljc")
((cljr--cljs-file-p)
"cljs")
((cljr--clj-file-p)
"clj"))
(when (cljr--point-in-reader-conditional-p)
(cond ((cljr--point-in-reader-conditional-branch-p :clj)
"clj")
((cljr--point-in-reader-conditional-branch-p :cljs)
"cljs")))))

(defun cljr--prompt-or-select-libspec (candidates)
"Prompts for namespace selection or returns only candidate.

This will auto-select the only candidate if `cljr-magic-requires'
is not set to `:prompt'."
(cond
((or (> (length candidates) 1) (eq :prompt cljr-magic-requires))
(completing-read "Add require: " candidates nil))
((= (length candidates) 1)
(seq-first candidates))))

(defun cljr--get-aliases-from-middleware ()
(when-let (aliases (cljr--call-middleware-for-namespace-aliases))
(if (cljr--clj-context-p)
Expand Down Expand Up @@ -2059,17 +2121,24 @@ will add the corresponding require statement to the ns form."
(not (cljr--in-number-p))
(clojure-find-ns)
(cljr--unresolved-alias-ref (cljr--ns-alias-at-point))))
(when-let (aliases (cljr--magic-requires-lookup-alias alias-ref))
(let ((short (cl-first aliases))
;; Ensure it's a list (and not a vector):
(candidates (mapcar 'identity (cl-second aliases))))
(when-let (long (cljr--prompt-user-for "Require " candidates))
(when (and (not (cljr--in-namespace-declaration-p (concat ":as " short "\b")))
(not (cljr--in-namespace-declaration-p (concat ":as-alias " short "\b")))
(or (not (eq :prompt cljr-magic-requires))
(not (> (length candidates) 1)) ; already prompted
(yes-or-no-p (format "Add %s :as %s to requires?" long short))))
(cljr--insert-require-libspec (format "[%s :as %s]" long short))))))))
(if cljr-slash-uses-suggest-libspec
(when-let (libspec
vemv marked this conversation as resolved.
Show resolved Hide resolved
(thread-first
alias-ref
(cljr--call-middleware-suggest-libspec (cljr--language-context-at-point))
cljr--prompt-or-select-libspec))
(cljr--insert-require-libspec libspec))
(when-let (aliases (cljr--magic-requires-lookup-alias alias-ref))
(let ((short (cl-first aliases))
;; Ensure it's a list (and not a vector):
(candidates (mapcar 'identity (cl-second aliases))))
(when-let (long (cljr--prompt-user-for "Require " candidates))
(when (and (not (cljr--in-namespace-declaration-p (concat ":as " short "\b")))
(not (cljr--in-namespace-declaration-p (concat ":as-alias " short "\b")))
(or (not (eq :prompt cljr-magic-requires))
(not (> (length candidates) 1)) ; already prompted
(yes-or-no-p (format "Add %s :as %s to requires?" long short))))
(cljr--insert-require-libspec (format "[%s :as %s]" long short)))))))))

(defun cljr--in-namespace-declaration-p (s)
(save-excursion
Expand Down
141 changes: 121 additions & 20 deletions tests/unit-test.el
Original file line number Diff line number Diff line change
Expand Up @@ -141,31 +141,132 @@

(describe "cljr--unresolved-alias-ref"
(it "returns unresolved alias reference"
(expect (cljr--with-clojure-temp-file "foo.clj"
(insert "(ns foo)")
(cljr--unresolved-alias-ref "ns-name"))
:to-equal "ns-name"))
(cljr--with-clojure-temp-file "foo.clj"
(insert "(ns foo)")
(expect (cljr--unresolved-alias-ref "ns-name")
:to-equal "ns-name")))

(it "returns nil for resolved alias"
(expect (cljr--with-clojure-temp-file "foo.clj"
(insert "(ns foo (:require [user.ns-name :as ns-name]))")
(cljr--unresolved-alias-ref "ns-name"))
:to-be nil))
(cljr--with-clojure-temp-file "foo.clj"
(insert "(ns foo (:require [user.ns-name :as ns-name]))")
(expect (cljr--unresolved-alias-ref "ns-name")
:to-be nil)))

(it "returns nil for js alias in cljs file."
(expect (cljr--with-clojure-temp-file "foo.cljs"
(insert "(ns foo)")
(cljr--unresolved-alias-ref "js"))
:to-be nil))
(cljr--with-clojure-temp-file "foo.cljs"
(insert "(ns foo)")
(expect (cljr--unresolved-alias-ref "js")
:to-be nil)))

(it "returns js for js alias in clj file."
(expect (cljr--with-clojure-temp-file "foo.clj"
(insert "(ns foo)")
(cljr--unresolved-alias-ref "js"))
:to-equal "js"))
(cljr--with-clojure-temp-file "foo.clj"
(insert "(ns foo)")
(expect (cljr--unresolved-alias-ref "js")
:to-equal "js")))

(it "returns js for js alias in cljc file."
(expect (cljr--with-clojure-temp-file "foo.cljc"
(insert "(ns foo)")
(cljr--unresolved-alias-ref "js"))
:to-equal "js")))
(cljr--with-clojure-temp-file "foo.cljc"
(insert "(ns foo)")
(expect (cljr--unresolved-alias-ref "js")
:to-equal "js"))))

(defmacro with-point-at (text &rest body)
(declare (indent 1))
`(progn (insert ,text)
(goto-char (point-min))
(re-search-forward "|")
dgtized marked this conversation as resolved.
Show resolved Hide resolved
(delete-char -1)
,@body))

(describe "cljr--language-context-at-point"
(it "identifies a clj file"
(cljr--with-clojure-temp-file "foo.clj"
(insert "(ns foo)")
(expect (cljr--language-context-at-point)
:to-equal '("clj" nil))))

(it "identifies a cljs file"
(cljr--with-clojure-temp-file "foo.cljs"
(insert "(ns foo)")
(expect (cljr--language-context-at-point)
:to-equal '("cljs" nil))))

(it "identifies a cljc file"
(cljr--with-clojure-temp-file "foo.cljc"
(insert "(ns foo)")
(expect (cljr--language-context-at-point)
:to-equal '("cljc" nil))))

(it "identifies a cljc file with a cljs context"
(cljr--with-clojure-temp-file "foo.cljc"
(with-point-at "(ns foo) #?(:cljs (Math/log|))"
vemv marked this conversation as resolved.
Show resolved Hide resolved
(expect (cljr--language-context-at-point)
:to-equal '("cljc" "cljs")))))

(it "identifies a cljc file with a clj context"
(cljr--with-clojure-temp-file "foo.cljc"
(with-point-at "(ns foo) #?(:clj (Math/log|))"
(expect (cljr--language-context-at-point)
:to-equal '("cljc" "clj"))))))

(describe "cljr--prompt-or-select-libspec"
(it "prompts user for namespace selection"
(spy-on 'completing-read :and-return-value "[a.a :as a]")
(expect (cljr--prompt-or-select-libspec '("[a.a :as a]"
"[a.b :as b]"))
:to-equal "[a.a :as a]"))

(it "returns user content from prompt regardless of candidates"
(spy-on 'completing-read :and-return-value "[alpha :as a]")
(expect (cljr--prompt-or-select-libspec
'("[a.a :as a]"
"[a.b :as b]"))
:to-equal "[alpha :as a]"))

(it "short-circuits if only one candidate matches"
(expect 'completing-read :to-have-been-called-times 0)
vemv marked this conversation as resolved.
Show resolved Hide resolved
(expect (cljr--prompt-or-select-libspec '("[a.a :as a]"))
:to-equal "[a.a :as a]"))

(it "prompts anyway if `cljr-magic-requires' is `:prompt'"
(spy-on 'completing-read :and-return-value "[a.a :as a]")
(let ((cljr-magic-requires :prompt))
(expect (cljr--prompt-or-select-libspec '("[a.a :as a]"))
:to-equal "[a.a :as a]"))))

(describe "cljr-slash"
(before-each (setq cljr-slash-uses-suggest-libspec t))
(after-each (setq cljr-slash-uses-suggest-libspec nil))
vemv marked this conversation as resolved.
Show resolved Hide resolved
(it "inserts single selection from suggest-libspec"
(spy-on 'cljr--call-middleware-suggest-libspec
:and-return-value (parseedn-read-str "[\"[bar.alias :as alias]\"]"))
(cljr--with-clojure-temp-file "foo.cljc"
(with-point-at "(ns foo)\nalias|"
(cljr-slash))
(expect (buffer-string) :to-equal "(ns foo
(:require [bar.alias :as alias]))
alias/")))

(it "prompts user for selection from suggest-libspec"
dgtized marked this conversation as resolved.
Show resolved Hide resolved
(spy-on 'cljr--call-middleware-suggest-libspec
:and-return-value (parseedn-read-str "[\"[bar.example :as ex]\" \"[baz.example :as ex]\"]"))
(spy-on 'completing-read
:and-return-value "[baz.example :as ex]")
(cljr--with-clojure-temp-file "foo.cljc"
(with-point-at "(ns foo)\nex|"
(cljr-slash))
(expect (buffer-string) :to-equal "(ns foo
(:require [baz.example :as ex]))
ex/")))

(it "inserts modified user input from completing-read"
(spy-on 'cljr--call-middleware-suggest-libspec
:and-return-value (parseedn-read-str "[\"[bar.example :as ex]\" \"[baz.example :as ex]\"]"))
(spy-on 'completing-read
:and-return-value "[baz.example :as ex :refer [a b c] ]")
(cljr--with-clojure-temp-file "foo.cljc"
(with-point-at "(ns foo)\nex|"
(cljr-slash))
(expect (buffer-string) :to-equal "(ns foo
(:require [baz.example :as ex :refer [a b c] ]))
ex/"))))