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

hover: remove spacing between type definition #619

Merged
merged 3 commits into from
Nov 14, 2022

Conversation

aspeddro
Copy link
Contributor

Before:

Neovim LSP builtin:
image

VSCode:
image

After:

Neovim:
image

VSCode:
image

@zth
Copy link
Collaborator

zth commented Nov 8, 2022

Having a new line there is intentional for VSCode, but I do see the problem in neovim. We'll probably need to treat them differently.

@aspeddro
Copy link
Contributor Author

aspeddro commented Nov 8, 2022

Fixed by 502d591

VSCode:

image

Neovim:

image

@@ -97,7 +97,7 @@ let stringifyCompletionItem c =
| None -> null
| Some doc -> stringifyMarkupContent doc)

let stringifyHover s = Printf.sprintf {|{"contents": "%s"}|} (Json.escape s)
let stringifyHover value = Printf.sprintf {|{"contents": %s}|} (stringifyMarkupContent {kind = "markdown"; value})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use MarkupContent. MarkedString and MarkedString[] are depreacted.

@zth
Copy link
Collaborator

zth commented Nov 9, 2022

That looks good. What about the dividers, would you like to test removing them completely in vim too?

@aspeddro
Copy link
Contributor Author

aspeddro commented Nov 9, 2022

Neovim automatically adds dividers between code blocks even though it doesn't return dividers \n---\n

result = {
      contents = {
        kind = "markdown",
        value = "```rescript\n(\n  Belt.Array.t<Layer.t>,\n  Layer.t => string,\n) => Belt.Array.t<string>\n```\n```rescript\ntype Belt.Array.t<'a> = array<'a>\n```\n\n```rescript\ntype Layer.t = Alface | Tomate
| Queijo | Cebola | Pao\n```\n\n\n\n  `map(xs, f)`\n\n  Returns a new array by calling `f` for each element of `xs` from the beginning to end.\n\n  ```res example\n  Belt.Array.map([1, 2], (x) => x + 1) == [3, 4]
\n  ```\n"
      }
}

image

@aspeddro
Copy link
Contributor Author

aspeddro commented Nov 9, 2022

Dividers do indeed improve readability.

@aspeddro aspeddro force-pushed the fix-markdown-divider-render branch from e6a90df to 600016e Compare November 9, 2022 19:19
@@ -100,12 +100,12 @@ let hoverWithExpandedTypes ~docstring ~file ~package ~supportsMarkdownLinks typ
Markdown.goToDefinitionText ~env ~pos:loc.Warnings.loc_start
else ""
in
"\n" ^ Markdown.spacing
Markdown.divider ^ (if supportsMarkdownLinks then Markdown.spacing else "")
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably name supportsMarkdownLinks something smarter now that it handles more cases, but that's a question for later.

@zth
Copy link
Collaborator

zth commented Nov 13, 2022

@aspeddro ready to merge as soon as the tests are green again 👍

@aspeddro aspeddro force-pushed the fix-markdown-divider-render branch from 600016e to 814a710 Compare November 13, 2022 20:51
@aspeddro
Copy link
Contributor Author

I don't know why the tests are failing, all the changes are committed.

@zth
Copy link
Collaborator

zth commented Nov 13, 2022

Can you resolve the merge conflict here too? I'll have a look at the tests tomorrow.

@aspeddro
Copy link
Contributor Author

Done

@aspeddro aspeddro force-pushed the fix-markdown-divider-render branch from 3e95a89 to 66fa6b9 Compare November 14, 2022 08:56
@aspeddro
Copy link
Contributor Author

🙏 Tests ✅

@zth zth merged commit 8a3e5fb into rescript-lang:master Nov 14, 2022
@aspeddro aspeddro deleted the fix-markdown-divider-render branch December 19, 2022 17:26
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.

2 participants