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

feat: add detail to documentation window #33

Merged
merged 1 commit into from
Oct 10, 2024

Conversation

telebart
Copy link
Contributor

@telebart telebart commented Oct 8, 2024

Stylize the documentation window

Before:
Screenshot 2024-10-08 at 17 06 14
Screenshot 2024-10-08 at 17 06 02
After:
Screenshot 2024-10-08 at 17 06 36
Screenshot 2024-10-08 at 17 06 27

@julienvincent
Copy link
Contributor

julienvincent commented Oct 8, 2024

I have one gripe with this change:

I was using nvim-cmp before and it would constantly break trying to render documentation coming from clojure-lsp. The failure came from calling the stylize_markdown function you are using in this change. See example below:

image

If this lands I would suggest either:

  1. Catching exceptions from calling this (using pcall or something) and then falling back to the existing behaviour.
  2. Sticking it behind some config that can be turned off.

Otherwise, this looks awesome!

🙏🏻

@telebart
Copy link
Contributor Author

telebart commented Oct 8, 2024

Hmm, I guess having stylize_markdown behind pcall would be a sensible thing to do since this isn't probably the only edge case considering markdown and lsp standards.
I'm interested though what is causing it since the error seems quite straightforward nil exception and could probably be fixed in upstream to utils. Sadly I couldn't find a setup to reproduce that error with my limited knowledge of clojure.

@sudo-tee
Copy link

sudo-tee commented Oct 8, 2024

I was also trying to get this work and found 2 things that could help

you could check item.documentation.kind == "markdown" instead of a table.

The second part is for some reason if a code block does not end in it's own newline it will break vim.lsp.stylize_markdown
image

but if you replace the code with something like this doc = doc:gsub('```\n', '\n```\n') it works properly

image

@julienvincent
Copy link
Contributor

julienvincent commented Oct 8, 2024

Sadly I couldn't find a setup to reproduce that error with my limited knowledge of clojure.

To reproduce:

  1. Make sure you have clojure-lsp configured (the defaults from nvim-lspconfig work perfectly fine)
  2. Use this PR's branch
  3. Make and edit a clojure file called example.clj
  4. Add the following:
(ns example
  (:require
   [clojure.test :as t]))

(t/*testi|) ;; trigger autocomplete here
  1. Success :)

Triggering autocomplete there will give you an immediate failure. I know I have reproduced this in other languages in the past but I wasn't able to get a reliable reproduce sequence now. Sorry for the clojure :)

@telebart telebart force-pushed the prettify-documentation branch from 5fa7b06 to 2bfbc25 Compare October 9, 2024 05:09
@telebart
Copy link
Contributor Author

telebart commented Oct 9, 2024

I was also trying to get this work and found 2 things that could help

you could check item.documentation.kind == "markdown" instead of a table.

The second part is for some reason if a code block does not end in it's own newline it will break vim.lsp.stylize_markdown

Thanks for looking into this. This seems like a malformed value. I would not like to fix it here since it feels weird to do this kind of duct tape solutions when there is something wrong with upstream

  documentation = {
    kind = "markdown",
    value = "```lua\nlocal status, err_or_value = pcall(function)```\n---\nProtect call a function as a variable"
  }
5. Success :)

Couldn't succeed with failing but now that I think about it I might have seen the error message flash before with other languages.

Sorry for the clojure :)

It's more of a problem of loose standards and string parsing that is at fault here. Nothing to be sorry about using a language

I would propose that this PR would just try the stylizing (If it's wanted/needed feature) from the lsp utils library and falls back to simpler one. Other things would be out of scope and subject for another PR.

Added fallback and removed hanging printing statements.
Simple formatting:
Screenshot 2024-10-09 at 7 50 21

@telebart telebart force-pushed the prettify-documentation branch from 2bfbc25 to bee69be Compare October 10, 2024 07:29
@telebart
Copy link
Contributor Author

After testing this for a while I realized that I really only needed the item.detail for the documentation window.
Sorry for changing the scope.

@telebart telebart changed the title feat: stylize documentation window feat: add detail to documentation window Oct 10, 2024
@Saghen Saghen merged commit 588e4d4 into Saghen:main Oct 10, 2024
@Saghen
Copy link
Owner

Saghen commented Oct 10, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants