Setting ImplicitParagraph has no effect

[sklivvz]

As far as I understand, setting ImplicitParagraph to false should prevent markdig from wrapping rendered HTML in a <p>.

However, the following test does not behave as expected

            var pipeline = new MarkdownPipelineBuilder().Build();
            var writer = new StringWriter();
            var renderer = new Markdig.Renderers.HtmlRenderer(writer);
            renderer.ImplicitParagraph = false;
            pipeline.Setup(renderer);
            var foo = Markdown.ToHtml("test", pipeline);

            // foo is "<p>test</p>\n" and not "test"

Furthermore, this parameter (and the couple more similar) are only interesting at render time and as such, they should be directly settable -- the user should not be forced to go through 5 lines of setup to set a basic option, such as when rendering a snippet. This also makes them basically undiscoverable.

[xoofx]

Furthermore, this parameter (and the couple more similar) are only interesting at render time and as such, they should be directly settable

I don't remember the details of this particular parameter, but these parameters on the Renderer class are often render states used by the renderers and not always settable by end-user. So their discover-ability was absolutely not a concern, unless you want to develop a new custom renderer.

Furthermore, this parameter (and the couple more similar) are only interesting at render time and as such, they should be directly settable -- the user should not be forced to go through 5 lines of setup to set a basic option, such as when rendering a snippet. This also makes them basically undiscoverable.

Well, the situation could have been worse, I could have decided back in the days to make them completely internal. But at least, the API is giving you a chance to have an access for it, so that's not that bad right?

I'm no longer in the details of the API to propose anything right now, nor I have dedicated personal time to track this, so if you could take the time to make a proposal, and a PR, that would be helpful.

[MihaZupan]

ImplicitParagraph is false by default, if you don't want paragraph tags, set it to true.

Also Markdown.ToHtml creates its own renderer. If you create your own HtmlRenderer you have to render on it explicitly like so:

var pipeline = new MarkdownPipelineBuilder().Build();

var document = Markdown.Parse("test", pipeline);

StringWriter writer = new StringWriter();
var renderer = new HtmlRenderer(writer);
renderer.ImplicitParagraph = true; // note this change
pipeline.Setup(renderer);

renderer.Render(document); // using the renderer directly
writer.Flush();
string html = writer.ToString();
// test

the user should not be forced to go through 5 lines of setup to set a basic option

I agree it's not user-friendly rn, you are pretty much copying the implementation of ToHtml. I was thinking of adding another helper method, something like:

public class Markdown
{
    public static string ToHtml(string markdown, MarkdownPipeline pipeline, Action<HtmlRenderer> setupAction)
}

So this example would become

var pipeline = new MarkdownPipelineBuilder().Build();
string html = Markdown.ToHtml("test", pipeline, renderer => renderer.ImplicitParagraph = true);

[sklivvz]

OK, so first of all thanks for clarifying the problem and its solution. I agree that a helper method would improve the situation, perhaps with some extra docs.

This worked for me, although it needs to be moved to the right class, tested etc.

    public static class Mark {
        public static string ToHtml(string markdown, MarkdownPipeline pipeline, Action<HtmlRenderer> setupAction)
        {
            var writer = new StringWriter();
            var renderer = new Markdig.Renderers.HtmlRenderer(writer);
            setupAction(renderer);
            renderer.Render(Markdown.Parse(markdown));
            return writer.ToString();
        }
    }

If you like the approach I'll push a PR tomorrow

Thanks,

[MihaZupan]

If @xoofx agrees with the API shape, the implementation would probably be like so: (note that I changed the parameters)

Might be cleaner to reverse configureRenderer and pipeline and make pipeline non-default as I would imagine most callers will specify it and having an Action as a last parameter is usually more readable.

public static string ToHtml(string markdown, Action<HtmlRenderer> configureRenderer, MarkdownPipeline pipeline = null, MarkdownParserContext context = null)
{
    if (markdown is null) throw new ArgumentNullException(nameof(markdown));
    if (configureRenderer is null) throw new ArgumentNullException(nameof(configureRenderer));

    pipeline ??= new MarkdownPipelineBuilder().Build();
    pipeline = CheckForSelfPipeline(pipeline, markdown);

    var writer = new StringWriter();
    var renderer = new HtmlRenderer(writer);
    pipeline.Setup(renderer);

    configureRenderer(renderer);

    var document = Parse(markdown, pipeline, context);
    renderer.Render(document);
    writer.Flush();

    return writer.ToString();
}

[audigex]

I feel like this is all massively overcomplicated for the end user, for something that seems so basic and fundamental. Clearly there are a lot of situations where the user is going to want to render markdown inline.

It seems like this is all just a very roundabout way of handling something that should be done with the existence of ToInlineHtml() or ToHtml(bool inline = false)

[MihaZupan]

I don't know if this specific case is so common that a dedicated overload would be necessary.

Most users requiring fine-grained control will likely want to adjust more than one thing and will already be doing the pipeline setup and rendering steps manually.

[kinetiq]

@MihaZupan For what it's worth, I need it and I'm frustrated. I imagine this must be very common (especially considering the upvotes on the comment asking for an overload).

I've been poking around trying to find a solution that isn't an overblown hack but I'm giving up - I'm going to go do it the ugly way. :)

[xoofx]

@MihaZupan For what it's worth, I need it and I'm frustrated. I imagine this must be very common (especially considering the upvotes on the comment asking for an overload).

Let me reiterate from my original response, that if someone wants such a feature, this person will have to make a PR for it with tests. 🙂