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

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

Open
StartAutomating opened this issue Feb 23, 2024 · 6 comments

Comments

@StartAutomating
Copy link

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.

@mklement0
Copy link

mklement0 commented Feb 23, 2024

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;
  }

@StartAutomating
Copy link
Author

@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 ;-)

@mklement0
Copy link

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.

@StartAutomating
Copy link
Author

@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 pushed a commit to StartAutomating/MarkdownRender that referenced this issue Feb 23, 2024
…Shell#28 )

If HTML is not present, it should be  or VT100EncodedString.
@StartAutomating
Copy link
Author

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

PR updated. Thanks for the feedback!

StartAutomating pushed a commit to StartAutomating/MarkdownRender that referenced this issue Feb 23, 2024
@mklement0

This comment was marked as resolved.

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

No branches or pull requests

2 participants