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

Render hover documentation as Markdown in LSP #1559

Merged
merged 5 commits into from
Sep 4, 2023

Conversation

deotimedev
Copy link
Contributor

@deotimedev deotimedev commented Aug 30, 2023

Renders hover documentation as markdown instead of plaintext in LSP

Before:

image

After:

image

@deotimedev deotimedev changed the title Render documentation as Markdown in LSP Render hover documentation as Markdown in LSP Aug 30, 2023
MarkedString::String(
meta.iter()
.flat_map(|s| s.lines())
.map(|s| s.trim())
Copy link
Member

Choose a reason for hiding this comment

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

This seems to trim the identation from code snippets, which is probably not intended.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, it seems like the desired behavior should be like that of the indoc! macro which removes only the common indentation in the lines. Should I add the implementation for that macro, unindent, as a dependency, or make a seperate function for that?

Copy link
Member

Choose a reason for hiding this comment

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

Since unindent is in our dependency tree already for indoc, using that seems like a reasonable thing to do 👍

meta.iter()
.map(|s| {
s.lines()
.map(|s| if s.is_empty() { " " } else { s })
Copy link
Contributor Author

@deotimedev deotimedev Aug 30, 2023

Choose a reason for hiding this comment

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

Fixes a strange regression with the unindent crate that causes strings like this to be unindented properly:
image

But not this:
image

Copy link
Member

Choose a reason for hiding this comment

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

Could be worth adding this information directly in the code, via a comment.

Copy link
Member

@yannham yannham left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! However, I'm a bit confused as to why we need to unindent here. The doc is parsed without the common indentation prefix (which I just checked by println! the raw documentation of std.contract.apply after parsing), that is, Nickel multiline strings should have the same effect as unindent already. Do you have any idea where does this indentation come from?

@deotimedev
Copy link
Contributor Author

I see here that multiline strings should be getting parsed with indent removed:
image
However, it still seems like the indent remains in these strings anyways:
image
image

Not entirely sure why this is happening, also found this strip_indent_doc function, but it doesn't seem like its being used anywhere currently
image

@deotimedev
Copy link
Contributor Author

Also seems like the documentation created from the nickel doc command keeps the indent in it:
image
image

@vkleen
Copy link
Member

vkleen commented Aug 31, 2023

I'm tentatively suspecting pretty sure that's a line ending issue and should be fixed in the multiline string parser. With \n line endings the nickel doc output is correctly unindented for me.

@yannham
Copy link
Member

yannham commented Sep 1, 2023

Ah, \r\n is probably not considered an empty line, and thus the indentation prefix is set to 0 because of this. This is why we need more Windows tester 🙃 Then this is a bug in Nickel itself, and the LSP shouldn't try to handle this. @DeoTimeTheGithubUser, would that be ok to get rid of unindent here? We'll take care of the issue in the parsing of multiline strings separately.

@yannham
Copy link
Member

yannham commented Sep 4, 2023

Perfect, many thanks!

@yannham yannham added this pull request to the merge queue Sep 4, 2023
Merged via the queue into tweag:master with commit 0080bf5 Sep 4, 2023
4 checks passed
suimong pushed a commit to suimong/nickel that referenced this pull request Sep 17, 2023
* Render documentation as markdown in LSP

* Use unindent crate to retain code indentation

* Render contracts seperate from documentation

* Remove usages of unindent

* Remove unindent from `Cargo.lock`

---------

Co-authored-by: BuildTools <[email protected]>
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.

None yet

3 participants