Markdig version update

[lyndaidaii]

Update markdig with latest version to have all new features

• Update markdig to latest version 0.22.0

• New extension added:
  table, tasklist, autolink, emoji

• Feature flag was added

Below showed the table, autolink, emoji render difference with two libs
Old CommonMark render:

New Markdig render:

Details comparison about Markdig and CommonMark, please check out OneNote: Markdig vs CommonMark

Addresses https://github.com/NuGet/Engineering/issues/3429

[loic-sharma]

What's the update for? FYI, the issue link seems to be incorrect

[lyndaidaii]

Tested with updated markdig version in serverCommon, need update package version with NuGet.Services.Messaging.Email, then merge this after change in serverCommon(NuGet/ServerCommon#348)

[joelverhagen]

Please verify the account deletion emails in particular. The account deleter is in the strange position to refer to ServerCommon, NuGet.Jobs, and NuGetGallery libraries all at once. There can be some strange dependency issues because of that.

[lyndaidaii]

Would have separate PR for migrating functionality using markdig, I will revert the last commit "migrate to markdig" after test

[cristinamanum]

Any argument validation needed?

[cristinamanum]

This method seems to be used only here. Is the parametrization of the "incrementHeadersBy" needed?

[lyndaidaii]

Current we support adaptive header, in order to continue support the same functionality, I believe we still need "incrementHeaderBy"

[cristinamanum]

Maybe add a comment why 6?

[zhhyu]

Should this comment be used for links? I see that we always apply HTTPS for images, even though I am not sure why we design it in this way.

[lyndaidaii]

Yes, It's also used to link. I could make same comments for links if you think it's not clear

[zhhyu]

I think this comment only applies to links (see "CommonMark" one). For images, we don't "allow http or https", and we don't transform links to https only for known domains. We always force images to use "https", is it ("rewriteAllHttp: true")?

[zhhyu]

I am not very sure but it may be helpful to have a test case of "Nested Blockquotes".

[lyndaidaii]

Nested Blockquotes added with latest changes

[zhhyu]

nit: how about naming it like "ThrowsArgumentOutOfRangeExceptionForNegativeIncrementHeadersBy()"?

[zhhyu]

nit: maybe add a message about the accepted range, for example:

NuGetGallery/src/NuGetGallery/Helpers/StreamHelper.cs

Lines 28 to 31 in e53e623

	if (maxSize <= 0 || maxSize >= int.MaxValue)
	{
	throw new ArgumentOutOfRangeException(nameof(maxSize), $"{nameof(maxSize)} must be greater than 0 and less than int.MaxValue");
	}

and you can also verify this message in the test.

[lyndaidaii]

Added changes with more details message for exception

NuGetGallery/src/NuGetGallery/Services/MarkdownService.cs

Line 60 in a0ef14f

throw new ArgumentOutOfRangeException(nameof(incrementHeadersBy), $"{nameof(incrementHeadersBy)} must be greater than or equal to 0");

test:

NuGetGallery/tests/NuGetGallery.Facts/Services/MarkdownServiceFacts.cs

Line 27 in a0ef14f

public void ThrowsArgumentOutOfRangeExceptionForNegativeIncrementHeadersBy(int incrementHeadersBy)

            [Theory]
            [InlineData(-1)]
            public void ThrowsArgumentOutOfRangeExceptionForNegativeIncrementHeadersBy(int incrementHeadersBy)
            {
                var exception = Assert.Throws<ArgumentOutOfRangeException>(() => _markdownService.GetHtmlFromMarkdown("markdown file test", incrementHeadersBy));
                Assert.Equal(nameof(incrementHeadersBy), exception.ParamName);
                Assert.Contains("must be greater than or equal to 0", exception.Message);
            }