-
-
Notifications
You must be signed in to change notification settings - Fork 223
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
Add ClojureDocs Command for examples to Rich Comments and to Output window #1363
Conversation
I think we should not worry about that for a first version of this, or maybe ever, but we can see how it goes. This is great work! |
4bf534e
to
7d33d62
Compare
@@ -397,6 +398,18 @@ async function openLogFile(): Promise<void> { | |||
} | |||
} | |||
|
|||
export async function getClojuredocs(symName: string, symNs: string): Promise<any> { | |||
if (serverVersion > '2021.10.20-16.49.47') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this >
operator work when checking against a string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that even if this check worked as it looks it was intended, there's a chance that null
would be returned from this function just because the version was not a desired/capable version, and then the message here would be misleading, because clojureDocs
would be null
but symbol maybe would exist. It would say Only Clojure core (ish) symbols supported
when really the version was not a desired/capable version.
I may have missed something, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ”dictionary” order, character by character. https://javascript.info/comparison#string-comparison This type of check works because clojure-lsp has the kind of version string that it has. For other schemes, like Calva's, we would rather bring in a semver library, I think.
As for the message: good catch! I originally wrote that for when there were only direct commands fetching clojuredocs. Now when it happens on hover, it makes no sense to show a message. And we don't actually, so probably the message
field should be removed. Which actually puts the whole NoDocsEntry
construct into question (that might have been a bad idea all the time, I don't know 😄).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thanks for educating me on that comparison!
WIP, addressing #689
Adding a command for testing how to get clojuredocs. (Which can't be done yet, see: clojure-lsp/clojure-lsp#598) Addressing #689
c866a45
to
7bc44d2
Compare
What has Changed?
Reach Clojuredocs examples quickly.
Adding:
ctrl+alt+r d
(to go well with the command for opening a new rich comment block).ctrl+alt+o d
Here's a demo of the RFC one:
clojuredocs-sfc-surf.mp4
Also showing examples in hovers, with button at each example to print it as a rich comment. (Print to REPL window TBD.)
clojuredocs-hover-rfc.mp4
Please also consider retweeting:
There are some quirks with this:
Fixes #689
My Calva PR Checklist
I have:
dev
branch. (Or have specific reasons to target some other branch.)published
. (Sorry for the nagging.)[Unreleased]
entry inCHANGELOG.md
, linking the issue(s) that the PR is addressing.ci/circleci: build
test. NB: You need to sign in/up at Circle CI to find the Artifacts tab.Ping @PEZ, @bpringe