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

🐛 fix markdown formatting for hover lsp #175

Merged
merged 1 commit into from
Feb 25, 2024

Conversation

AucaCoyan
Copy link
Contributor

@AucaCoyan AucaCoyan commented Feb 25, 2024

Hi, I wandered around for a bit and found out that when the server returns a hover, you have to inform the client which type of plaintext | markdown you are returning.

So, instead of

      const contents = {
        value: obj.hover, // actual hover string
        language: "nushell",
      };

it should be

      const contents = {
        value: obj.hover, // actual hover string
        kind: "markdown"
      };

(note that both key and value changed)

and you get the result 😄
image

By removing the language: nushell key, it defeats me how vscode knows that the code block (```) parts of the hover are in nushell syntax, because that is not written in the raw hover string 🧠 .

I hope this doesn't break any other editor that doesn't have markdown syntax!

@fdncred
Copy link
Collaborator

fdncred commented Feb 25, 2024

Thanks @AucaCoyan! I had 2 thoughts myself on this.

  1. was the ``` were in the wrong place
  2. markdown just like this PR. (i was looking at this https://stackoverflow.com/questions/72002634/markdown-not-working-in-hover-content-in-lsp)

I guess it knows the language by the file type you have loaded maybe? Nice work!

@fdncred fdncred merged commit 6cb893d into nushell:main Feb 25, 2024
1 check passed
@AucaCoyan
Copy link
Contributor Author

AucaCoyan commented Feb 25, 2024

Yeah, I followed up those same steps, when 1st didn't solved the problem, I checked that markdown type of response

@AucaCoyan AucaCoyan deleted the fix-markdown-hover branch February 25, 2024 14:51
@fdncred
Copy link
Collaborator

fdncred commented Mar 7, 2024

I'm guessing there's still some problem with this because this is what I see on my mac this morning.
Screenshot 2024-03-07 at 6 34 05 AM

I'm wondering if there's some line ending thing that is causing this to not work right. I'll try to check on linux and windows later.

@fdncred
Copy link
Collaborator

fdncred commented Mar 7, 2024

hmmm, still looks broken in Ubuntu as well.
Screenshot 2024-03-07 at 6 55 58 AM

@fdncred
Copy link
Collaborator

fdncred commented Mar 7, 2024

ugh, windows too. I wonder what the deal is? it was working.
image

@bfren
Copy link

bfren commented Mar 21, 2024

I know this is merged, but I'm getting this on my machine (Windows) - and have been for a while - is there a fix?

@fdncred
Copy link
Collaborator

fdncred commented Mar 21, 2024

@AucaCoyan was going to look into it last we talked. I assume he just hasn't got around to it yet.

@AucaCoyan
Copy link
Contributor Author

AucaCoyan commented Mar 21, 2024

Hello! I tried some simple stuff, like removing the 3 backticks (`) from the ide hover and it didn't solved the problem. From there, I didn't had the chance to investigate further

@bfren
Copy link

bfren commented Mar 21, 2024

Do we know what actually broke it / which release it broke for?

@fdncred
Copy link
Collaborator

fdncred commented Mar 21, 2024

Do we know what actually broke it / which release it broke for?

Someone may have to do a git bisect to see what broke it. Either that or just play around with it until they show up right. My guess is that either the backticks are messed up or the error type is somehow set to string, even though we're calling it markdown.

@AucaCoyan
Copy link
Contributor Author

AucaCoyan commented Mar 21, 2024

Yep, I can do that the next time I sit with that problem. I will make time this weekend 😄
If anyone else is curious, bear in mind that vscode-nushell-lang serves from nushell, so maybe the markdown can be broken not by changes of this repo, but for changes in the nu --ide-hover from nushell/nushell. We have a two (or more) headed hydra here

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.

3 participants