fix Markdown output to escape pipe character

[bakermo]

Hello! Looking to get my feet wet contributing to open source. I've made changes here that should fix #1839.

This will escape the pipe | character in markdown table cells by replacing it with \| so that the tables render properly.
Example before escaping:

And after:

[dnfadmin]

All CLA requirements met.

[adamsitnik]

@bakermo big thanks for your contribution! PTAL at my comments

[adamsitnik]

I can see that you are not setting the value of this property to true for StackOverflow, but for some reason the SO exporter is escaping it:

[MarkdownExporterAttribute.Default]
[MarkdownExporterAttribute.GitHub]
[MarkdownExporterAttribute.StackOverflow]
[MarkdownExporterAttribute.Atlassian]
public class Intro_MethodsParamsEscaped
{
    [Params("Should|Escape", "ShouldNotEscape")] public string Param;

    [Benchmark] public bool Method() => Param is null;
}

dotnet run -c Release -f net5.0 --filter *Intro_MethodsParamsEscaped* --job dry

Could you please ensure that it does not get escaped for the SO exporter?

[bakermo]

Ok, this should be fixed now.

Do we want to apply this escaping to console output? It would render as \| in the console, which is probably not desired, however there are comments in the code indicating that we want to support copy-paste from the console into Github:

BenchmarkDotNet/src/BenchmarkDotNet/Exporters/MarkdownExporter.cs

Line 37 in 67bf3c0

ColumnsStartWithSeparator = true // we want to be able to copy-paste the console output to GH #1062

[adamsitnik]

big thanks for adding a new approval test! 👍

[adamsitnik]

BTW are there any other characters that should be escaped?

[bakermo]

@adamsitnik Appreciate the feedback!

BTW are there any other characters that should be escaped?

From what I've seen from testing, the pipe character is the only one that breaks output, but I will make another pass to confirm this after addressing your other comments later today.

[YegorStepanov]

@bakermo I think there should be EscapeSpecialCharacters too. Is it should escape all characters? \t too?

Try run this (\a is a sound character, and it makes sounds!):

public class Benchmarks
{
    [Params("Shou\nld|Esca\ape")] public string Param;

    [Benchmark] public bool Method() => Param is null;
}

[bakermo]

@YegorStepanov Ah, good catch. Yes, I agree.

Do you think it would be best to have a single flag EscapeSpecialCharacters that includes the pipe, or keep them separate (EscapeSpecialCharacters and EscapePipe)?

My inclination is to keep them separate, as EscapeSpecialCharacters could handle all characters that we probably want escaped across multiple (or all) outputs, and leave EscapePipe specific to the outputs that care about it. That, or each export dialect defines its own character set that it needs escaped. Thoughts?

[YegorStepanov]

@bakermo My opinion is exactly the same as yours. Separate them.

I don't remember any other markdown-specific mistakes in the output, escapePipe is enough currently.

By implementing it you will close one or few other issues. You may find them and link to the PR.

[YegorStepanov]

👍

Have you corrected all Adam's comments?

It's not so easy to escape | in the console, because the table alignment will be broken. To fix it the big refactoring of the markdown exporter is required.

We will need to calculate the max column width and only after it, print the table.

In #2135 I've escaped the control characters (like '\0', '\a' - it's still making sound :))

@adamsitnik

[YegorStepanov]

The big refactoring will also fix raw markdown alignment when using bold text:

(the PR correctly escapes '|', the screenshots below from master branch)

Current:

Should be: