MarkdownRender icon indicating copy to clipboard operation
MarkdownRender copied to clipboard

Microsoft.PowerShell.MarkdownRender.MarkdownInfo.ToString() should be the HTML

Open StartAutomating opened this issue 1 year ago • 6 comments

Summary of the new feature / enhancement

As a user, I want to be able to more seamlessly work with Markdown in PowerShell.

Unfortunately, when I try to ToString() a Markdown object, I get:

Microsoft.PowerShell.MarkdownRender.MarkdownInfo

This makes easily embedding markdown inside of a string more painful than it needs to be, and also complicates logic for -joining multiple markdowns

Proposed technical implementation details (optional)

The .ToString() method the MarkdownInfo class should be the converted HTML.

StartAutomating avatar Feb 23 '24 19:02 StartAutomating

While I think it's a good idea to have .ToString() return a meaningful representation, note that the logic of the Microsoft.PowerShell.MarkdownRender.MarkdownInfo-returning ConvertFrom-Markdown cmdlet is to either fill in the .Html (by default) or the .VT100EncodedString property (if requested via -AsVT100EncodedString).

Thus, it's probably better to do something like [operand order updated after feedback]:

  public override string ToString() {
    return this.Html ?? this.VT100EncodedString ?? string.Empty;
  }

mklement0 avatar Feb 23 '24 20:02 mklement0

@mklement0 I'm definitely open to the idea of a flag, as I think both are useful.

However / and, I think that should be the job of a local formatter (which should probably call Show-Markdown)

I think HTML construction is the more common / intended use case and should be the default.

I also think ToString() could have multiple overloads ;-)

StartAutomating avatar Feb 23 '24 20:02 StartAutomating

I think HTML construction is the more common / intended use case and should be the default.

It is the default, but the problem is that .Html isn't guaranteed to have a value, and if it doesn't (if -AsVT100EncodedString was used), with your approach would it would string-interpolate to the empty string - and I don't think that's desirable.

Defaulting to whichever property is filled in to me seems like the preferable behavior.

As for multiple overloads: That would be useful in the abstract - say .ToString(MarkdownConversionType.Html), .ToString(MarkdownConversionType.VT100), and (not currently an option) .ToString(MarkdownConversionType.PlainText) - but at least currently makes no sense, as long as only ever one property is filled in.

From what I can tell, while you can currently publicly instantiate a MarkdownInfo instance, you cannot fill it yourself, and therefore have no way of requesting that both .Html and .VT100EncodedString be filled - possibly on demand.

mklement0 avatar Feb 23 '24 20:02 mklement0

@mklement0 I get what you're saying here, and I think the best approach would be to make the ToString() conditional depending on which had content.

StartAutomating avatar Feb 23 '24 21:02 StartAutomating

Defaulting to whichever property is filled in to me seems like the preferable behavior.

PR updated. Thanks for the feedback!

StartAutomating avatar Feb 23 '24 21:02 StartAutomating

Glad to hear it, @StartAutomating - in my earlier comment I had the operand order backwards (though it would have worked as along as the either-or logic of the properties being filled in is in place), so I've updated it to avoid confusion.

mklement0 avatar Feb 23 '24 21:02 mklement0