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

Completion and documentation with leading literals #97

Merged
merged 1 commit into from
Jun 22, 2023

Conversation

eval
Copy link
Contributor

@eval eval commented Jun 22, 2023

Addresses 1, 2 and 3 as described in #95

Copy link
Owner

@alexander-yakushev alexander-yakushev left a comment

Choose a reason for hiding this comment

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

Great start! Left a few comments.

@@ -94,11 +95,12 @@
;; If prefix doesn't contain a period, using fuziness produces too many
;; irrelevant candidates.
(for [{:keys [^String ns-str, ^String file]} (utils/namespaces&files-on-classpath)
:let [[_ quoted prefix] (re-matches #"(@|@?#'|')?(.+)" prefix)]

Choose a reason for hiding this comment

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

Three usages of a not-so-trivial regex, I think this can be extracted into a function.

@@ -115,7 +117,8 @@

(defn doc [ns-or-class-str curr-ns]
(when (nscl-symbol? ns-or-class-str)
(let [ns-or-class-sym (symbol ns-or-class-str)]
(let [strip-literals #(->> % (re-find #"^[@#']*(.+)") last)

Choose a reason for hiding this comment

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

Two usages, can be extracted into the utils namespace too.

\"@#'some-alias/db\" => #'some.full.ns/db
"
[symbol-str ns]
(let [strip-literals #(->> % (re-find #"^[@#']*(.+)") last)

Choose a reason for hiding this comment

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

The code here allows multiple @ while the code above doesn't. Regardless of which way you want it, I'd rather have it consistent across all code that does this.

symbol-ns (when alias-or-ns-str
(str (or (find-ns (symbol alias-or-ns-str))
(get (ns-aliases ns) (symbol alias-or-ns-str)))))]
(ns-resolve ns (symbol symbol-ns symbol-name-str))))

Choose a reason for hiding this comment

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

I wonder how much of this has to be done manually since ns-resolve already handles a lot of cases. E.g.:

user=> (ns-resolve *ns* (symbol "set/difference"))
#'clojure.set/difference
user=> (ns-resolve *ns* (symbol "clojure.set/difference"))
#'clojure.set/difference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woha!, stripping lots of code 👍🏻

@eval
Copy link
Contributor Author

eval commented Jun 22, 2023

@alexander-yakushev really appreciate the quick feedback (and the new release), thanks! Made the changes.

Copy link
Owner

@alexander-yakushev alexander-yakushev left a comment

Choose a reason for hiding this comment

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

Just a few more 😅.

(src/doc "#'clojure.core/map" (-ns)) => (checker string?)
(src/doc "#'src/var-symbol?" (-ns)) => (checker string?)
(src/doc "bogus" (-ns)) => nil
(src/doc "bo/gus" (-ns)) => nil))

Choose a reason for hiding this comment

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

Could you please indent-2 these?

\"nothing-todo\" => '(nil \"nothing-todo\")
"
[symbol-str]
(rest (re-matches #"(@{0,2}#'|'|@)?(.+)" symbol-str)))
Copy link
Owner

@alexander-yakushev alexander-yakushev Jun 22, 2023

Choose a reason for hiding this comment

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

next would work better here because (next nil) => nil but (rest nil) => (). That way it would stay more consistent with re-matches returns.

Choose a reason for hiding this comment

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

OTOH, it can't really not match (except on an empty string), so it doesn't matter.

@@ -115,7 +117,8 @@

(defn doc [ns-or-class-str curr-ns]
(when (nscl-symbol? ns-or-class-str)
(let [ns-or-class-sym (symbol ns-or-class-str)]
(let [strip-literals #(-> % utils/split-by-leading-literals last)

Choose a reason for hiding this comment

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

I think second instead of last would be a more common way to refer to a tuple part. last to me implies a variadic collection rather than a tuple.

Also, (comp second utils/split-by-leading-literals) would look more idiomatic.

Namespace completion works on inputs with leading literals, e.g.
"@clojure.", "#'clojure.", "'some.alias".

Documentation works on inputs with leading literals:
"#'clojure.core", "'some-alias" "#'clojure.core/map"
@alexander-yakushev
Copy link
Owner

Awesome! Thank you for coming up with all these improvements!

If you want to tackle private completion for #' too, feel free to open another PR:).

@alexander-yakushev alexander-yakushev merged commit ba4f21b into alexander-yakushev:master Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants