Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Added MarkdownString type, Deprecated MarkedString #216 #238

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BugDiver
Copy link

Added interface MarkupContent for backword compatibility

Added interface MarkupContent for backword compatibility
@BugDiver
Copy link
Author

Can someone please check why the build is broken. It's passing for appveyor. Two out of three builds are passing for travis as well. Maybe retriggering fixes that.

Any other comments/feedback?

@keegancsmith
Copy link
Member

It triggered a data race which I haven't seen before! I filed #244 but it looks very much unrelated to your code. Restarted the CI job. Thanks for the ping.

@keegancsmith
Copy link
Member

Thanks, I just read the spec for MarkupContent. It seems a hover can now return

contents: MarkedString | MarkedString[] | MarkupContent

and that MarkupContent is somewhat equivalent to

type MarkupContent struct {
  Kind string // 'plaintext' | 'markdown'
  Value string
}

I'm almost tempted to drop support for the deprecated MarkedString to simplify. But that would likely be painful in practice. So we should adjust this PR so there is a clear path forward for deprecating. There isn't a nice way to convert MarkedString[] into MarkupContent while still remaining correct, so I think a type with an awful name like MarkedStringOrMarkupContent is unfortunately the way forward, then we can fully remove MarkedString support at some point.

Also your PR supports raw strings being converted into MarkupContent which isn't needed. It also has a field called IsTrusted which I don't see in the spec.

Copy link
Member

@keegancsmith keegancsmith left a comment

Choose a reason for hiding this comment

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

See recent comment.

@ianlopshire
Copy link
Contributor

What is preventing MarkedString[] from being converted to MarkupContent?

The MarkedString spec doesn't differentiate between markdown and plaintext so we can just default to markdown when converting.

// untested example
func MarkedStringsToMarkupContent(ms []MarkedString) (MarkupContent) {
	var b strings.Builder
	for i := range ms {
		if ms[i].isRawString {
			b.WriteString(ms[i].Value)
			continue
		}
		b.WriteString("```")
		b.WriteString(ms[i].Language)
                b.WriteString("\n")
		b.WriteString(ms[i].Value)
                b.WriteString("\n")
		b.WriteString("```")
		
		if i != len(ms) -1 {
			b.WriteString("\n")
		}
	}

	return MarkupContent{
		Kind: "markdown",
		Value: b.String()
	}
}

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

Successfully merging this pull request may close these issues.

3 participants