Use MarkdownString to format docstring

[stefan-toubia]

What has Changed?

• Format hover docstrings with MarkdownString
• Format completion docstring
• Add docstring to signature information

Currently multi-line docstrings are very hard to read because VS Code doesn't respect newlines. I think this is behavior that should be fixed with VS Code, but this PR fixes this by using MarkdownStrings to format the hover text.

The fix was possible by formatting the docstring as a codeblock, which preserves most of the docstring formatting. One thing to note is that this does seem to make my font a little bigger in the docstring portion, but I think this is fine since it is much more important to properly format the docstring.

Hover

Before

After

Embedded codeblock detection

Signatures

I tested out adding the docstring to the signature since I think this is when the docstring is most useful, I'm not sure if others have a different opinion on this.

Signature formatting is sometimes broken for long lines of text

Completion

Issues

• Because clojure docstrings are formatted with all lines aligned with the start column of the first line, the extra whitespace is carried over to the docstrings.

(defn testfn-2
  "Multiline docstrings
   Will show in the signature window."
  ([a] (println a))
  ([a b] (println a b)))

• The markdown header has a very large top padding

My Calva PR Checklist

I have:

• Read How to Contribute.
• Made sure I am directing this pull request at the dev branch. (Or have specific reasons to target some other branch.)
• Made sure I am changed the default PR base branch, so that it is not master. (Sorry for the nagging.)
• Tested the VSIX built from the PR (well, if this is a PR that changes the source code.) You'll find the artifacts by clicking Show all checks in the CI section of the PR page, and then Details on the ci/circleci: build test. (For now you'll need to opt in to the CircleCI New Experience UI to see the Artifacts tab, because bug.)
  • Tested the particular change
  • Figured if the change might have some side effects and tested those as well.
  • Smoke tested the extension as such.
• Updated the [Unreleased] entry in CHANGELOG.md, linking the issue(s) that the PR is addressing.

The Calva Team PR Checklist:

Before merging we (at least one of us) have:

• Made sure the PR is directed at the dev branch (unless reasons).
• Read the source changes.
• Given feedback and guidance on source changes, if needed. (Please consider noting extra nice stuff as well.)
• Tested the VSIX built from the PR (well, if this is a PR that changes the source code.)
  • Tested the particular change
  • Figured if the change might have some side effects and tested those as well.
  • Smoke tested the extension as such.
• If need be, had a chat within the team about particular changes.

Ping @PEZ, @kstehn, @cfehse, @bpringe

[PEZ]

Cool! Here are some quick thoughts, after having tested it very briefly.

• Things get much more readable, and I think this is the way to go.
• We need a way to detect when the docstring is formatted with Markdown (e.g. the (rum/defc) macro and deal with that somehow. Not sure how, but maybe just skip our markdown wrapping.
• While full docs in signature hovers make sense, they are a bit annoying already as it is...

About docs in signatures. If it is possible to know if the signature was brought up at will or just popping up because the user hasn't switched that behaviour off, then maybe it could make sense to include the docs in the case that the hover is explicitly requested. And, even then, probably make it configurable by the user.

[stefan-toubia]

I hadn't encountered markdown in docstrings before, that's good to know this is a thing. I added support for codeblocks in docstrings, but with the current behavior of the hover box I'm not sure that we would be able to support any arbitrary markdown syntax. Check out the new screenshot in the description.

I understand what you're saying about the signature, I took a quick look to see if your suggestion is possible and I didn't come up with any solutions. There is no command name for the Trigger Parameter Hints command, so I don't think there's a way to know if this was explicitly requested. I've removed it for now, I think I over estimated how worth it this addition was.

[PEZ]

After having run with it a day, I think I would configure it to be on in signatures if it was an option. I have automatic signature pop-ups disabled so for me they only show when i want it to. And to not have to hover over the function name to see the docs is actually pretty nice. Just sayn'.

[stefan-toubia]

Nice, I wasn't aware that you can disable automatic parameter hints. Sounds like a good plan, I will add it back and make it configurable.

[stefan-toubia]

I've found out some more information about how markdown is formatted.

In markdown, a new line needs two spaces in front of it. And in order to have whitespace at the start of a line, the   character needs to be used. Knowing this, it shouldn't be too hard to string replace these instances and be able to format the output without using a codeblock.

[stefan-toubia]

@PEZ I've made 2 changes

• Signature docstring is a configuration option (default true?)
• Fix the docstring extra space on line start

I've decided against trying to format the docstring without a codeblock. Beside the regex being a bit finicky, the biggest problem is that the plain text is not monospaced, so the alignment gets messed up no matter what.

I am good to go here, please review and play around. I guess the last question is what the default value for the added calva.showDocstringInParameterHelp option should be.

[PEZ]

I don't know how to get rid of the review request, so just typing something here. 😄

[stefan-toubia]

@PEZ I think you've pushed to the wrong branch 😅

[PEZ]

rum/defc docstrings look pretty awesome now!

The setting for including docs with parameter hints works well, I think. (I mean as a concept, even if it also seems to work well technically when I try it.) Even so I think it is a nice default to have it as false. I have on my todo to write something about VS Code and Calva setup and will include this thing there.

I think that when the docs are not included with the parameters, that it should not be a Markdown hover. They get visibly larger and line break more. So:

• Including doc -> Markdown
• Not including doc -> Plain hover

The padding at the top of the doc hover, nothing we can do about it, right?

[PEZ]

@PEZ I think you've pushed to the wrong branch 😅

Nah, I just merged dev onto this one for easier merging later.

[stefan-toubia]

@PEZ I think you've pushed to the wrong branch 😅

Nah, I just merged dev onto this one for easier merging later.

Ah, gotcha. Are you familiar with rebasing?

[PEZ]

I just copy/pasted the commands that Github suggested 😀

…

On Thu, Dec 5, 2019 at 7:53 PM Stefan Toubia ***@***.***> wrote: @PEZ <https://github.com/PEZ> I think you've pushed to the wrong branch 😅 Nah, I just merged dev onto this one for easier merging later. Ah, gotcha. Are you familiar with rebasing? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#503?email_source=notifications&email_token=AAAHKOW62CEDMJJHKF27R3DQXFE3LA5CNFSM4JVEHAJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGBXTCI#issuecomment-562264457>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAHKOTIDDOBECLSMTMO4J3QXFE3LANCNFSM4JVEHAJA> .

[stefan-toubia]

Default config value is now false and I was able to fix the top padding. Turns out it was my mistake, I added a header markup ### and had forgot to remove it. Much better now!

[stefan-toubia]

Ok squashed and good to go, LMK if you think anything else should be changed.

[PEZ]

Great. For me the only thing remaining would be to not use markdown in the case where doc strings are not included. Might see something else today, but right now that's the only thing on my wish list.

[stefan-toubia]

@PEZ I'm not sure I follow here, you're talking about this condition, right?

As far as I can see, there is no difference between this and the current appearance.

Is there something I'm missing?

[PEZ]

I was referring to the parameter hints. But even so it seems I saw things that weren't there. I'll merge this tomorrow when my brain is a bit rested.