Fix to preserve line break after headers issue #319 #321

BernieWhite · 2017-12-04T07:18:54Z

Updates to markdown parser and renderer to preserve line breaks after headers. Multiple line breaks are ignored.
Extra line break is removed when related links does not contain any links.
Test cases updated to test for preserving line breaks and when line breaks are absent.

Fix #319

…ine breaks related links

BernieWhite · 2017-12-04T07:24:35Z

@vors There is a fair bit in this PR. Any questions please let me know.

vors · 2017-12-05T15:54:20Z

I would need some time to review that

vors

Awesome, very solid approach and lots of tests, I like it a lot!

I left few comments about the code inline.
I also tried the scenario that I carried about the most: apply Update-MarkdownHelp 2 times and verify that second time didn't change anything.
This had a problem after the Example header: keeps adding 2 new lines every time.

Repro: run Update-MarkdownHelp ./docs/
Affected file: docs/Merge-MarkdownHelp.md

vors · 2017-12-06T05:28:32Z

src/Markdown.MAML/Model/Markdown/SectionBody.cs

+            return new SectionBody(text, formatOption);
+        }
+
+        public static implicit operator SectionBody(string text)


Let's avoid using implicit operator it makes reading the code too obscure

vors · 2017-12-06T05:28:48Z

src/Markdown.MAML/Model/Markdown/SectionBody.cs

+            return Text;
+        }
+
+        public static SectionBody New(string text, SectionFormatOption formatOption = SectionFormatOption.None)


Why not just overloaded ctor?

No worries. I'll do that.

vors · 2017-12-06T05:38:50Z

src/Markdown.MAML/Renderer/MamlRenderer.cs

@@ -87,6 +87,17 @@ private static IEnumerable<XElement> GenerateParagraphs(string text)
            return Enumerable.Empty<XElement>();
        }

+        private static IEnumerable<XElement> GenerateParagraphs(Markdown.MAML.Model.Markdown.SectionBody body)


SectionBody.FormatOptions is not used in this method. Should caller simple pass body to the existing method instead?

I had some occasions that ended up with a null SectionBody object. But using the null coalescing operator would probably be a better approach. Thanks, good comment.

vors · 2017-12-06T05:41:11Z

src/Markdown.MAML/Renderer/Markdownv2Renderer.cs

@@ -265,9 +267,17 @@ private string JoinWithComma(IEnumerable<string> args)
            return string.Join(", ", args);
        }

+        private bool ShouldBreak(SectionFormatOption formatOption)
+        {
+            return ((formatOption & SectionFormatOption.LineBreakAfterHeader) == SectionFormatOption.LineBreakAfterHeader);


Consider HasFlag instead for readability

vors · 2017-12-06T05:49:27Z

src/Markdown.MAML/Transformer/MamlModelMerger.cs

+                        {
+                            param.FormatOption = strParam.FormatOption;
+                        }
+                        catch (Exception ex)


When this can happen? It doesn't seem to protect against anything but strParam == null case which is better to spell out explicitly

Good pickup. A null reference of strParam is really the only exception that is likely to be raised however we are already checking for that condition before the block is processed so additional handling is not required.

vors · 2017-12-06T05:53:49Z

test/Markdown.MAML.Test/EndToEnd/EndToEndTests.cs

@@ -47,6 +48,105 @@ public void ProduceMultilineDescription()
            Assert.Equal(3, description.Length);
        }

+        [Fact]
+        public void PreserveMarkdownWhenUpdatingMarkdownHelp()


The test seems homogeneous so it only tests the extra line case. Let's mix line-break and no-line-break in one test for coverage.

Done. Updated test to also test for preserved formatting scenario used by Update-MarkdownHelp.

vors · 2017-12-06T05:54:40Z

test/Markdown.MAML.Test/Parser/ParserTests.cs

+            var documentNode = MarkdownStringToDocumentNode($"## DESCRIPTION\r\n\r\n{expected}");
+
+            var actual = (documentNode.Children.FirstOrDefault(node => node.NodeType == MarkdownNodeType.Paragraph) as ParagraphNode).Spans.FirstOrDefault().Text;
+            var hasLineBreak = (documentNode.Children.FirstOrDefault(node => node.NodeType == MarkdownNodeType.Heading) as HeadingNode).FormatOption == SectionFormatOption.LineBreakAfterHeader;


This is not readable, please break into few lines or subroutine.

vors · 2017-12-06T05:55:44Z

test/Markdown.MAML.Test/Parser/ParserTests.cs

+            Assert.Equal(expected, actual);
+            Assert.Equal(false, hasLineBreak);
+        }
+


Nice tests! Great coverage! 👏

vors · 2017-12-06T05:57:59Z

test/Markdown.MAML.Test/Renderer/MarkdownV2RendererTests.cs

+            {
+                Type = "String",
+                Name = "Path",
+                FormatOption = SectionFormatOption.LineBreakAfterHeader,


Maybe mix two styles together in this test and avoid the second one?

vors · 2017-12-06T06:00:52Z

test/Markdown.MAML.Test/Transformer/ParserAndTransformerTestsV2.cs

-            Assert.Equal(mamlCommand.Name, "Add-Member");
-            Assert.Equal(mamlCommand.Synopsis, "Adds custom properties and methods to an instance of a Windows PowerShell object.");
+            Assert.Equal("Add-Member", mamlCommand.Name);
+            Assert.Equal("Adds custom properties and methods to an instance of a Windows PowerShell object.", mamlCommand.Synopsis.Text);


Thanks for fixing these parameter order!
I made a mistake early in development and copy-pasted it everywhere

vors · 2017-12-06T06:11:04Z

Also, please don't rewrite history in this branch and just make commits on top, that would make subsequent reviews much easier.

BernieWhite · 2017-12-07T06:38:27Z

@vors Thanks. Unless I'm missing it, testing Update-MarkdownHelp using v0.8.3 still seems update Merge-MarkdownHelp.md with those extra lines.

Fairly sure I see the issue though, so I'll add it to the PR and update tests to catch it in the future.

vors · 2017-12-07T06:49:46Z

Oh, you are so right! That's pretty bad. If you can fix it as part of this PR that would be wonderful.

BernieWhite · 2017-12-07T08:15:58Z

All good. Update included.

vors

vors · 2017-12-08T07:33:36Z

src/Markdown.MAML/Renderer/Markdownv2Renderer.cs

@@ -269,7 +269,8 @@ private string JoinWithComma(IEnumerable<string> args)

        private bool ShouldBreak(SectionFormatOption formatOption)
        {
-            return ((formatOption & SectionFormatOption.LineBreakAfterHeader) == SectionFormatOption.LineBreakAfterHeader);
+            // If the line break flag is set return true.


nit: the code is pretty self explanatory, no need to this comment

vors · 2017-12-08T07:37:12Z

src/Markdown.MAML/Renderer/Markdownv2Renderer.cs

@@ -538,6 +539,12 @@ private void AddParagraphs(string body, bool noNewLines = false)
                body = GetAutoWrappingForMarkdown(paragraphs.Select(para => GetEscapedMarkdownText(para.Trim())).ToArray());
            }

+            // The the body already ended in a line break don't add extra lines on to the end
+            if (body.EndsWith("\r\n\r\n"))


vors · 2017-12-08T07:37:31Z

src/Markdown.MAML/Transformer/MamlModelMerger.cs

-                            Report($"    Exception {param.Name} FormatOption merge: \r\n{ex.Message}\r\n");
-                            _cmdletUpdated = true;
-                        }
+                        // Update the parameter with the merged in FormatOption


nit: no need for this comment

BernieWhite added 4 commits November 24, 2017 23:34

Updates to preserve blank line after header PowerShell#319

2b3018a

Updates to preserve blank line after header for examples and parameters

e462fb8

Updates to preserve parameter, input, output and to remove trailing l…

d1cd5d9

…ine breaks related links

Fixes PowerShell#319

b986e43

vors suggested changes Dec 6, 2017

View reviewed changes

Updates based on comments

f14027c

vors merged commit 16a5f68 into PowerShell:master Dec 8, 2017

vors reviewed Dec 8, 2017

View reviewed changes

BernieWhite deleted the bewhite/blank-line-issue-319 branch December 12, 2017 07:57

vors mentioned this pull request Mar 7, 2018

New-MarkdownHelp Generates Markdown that violates many MD rules #345

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix to preserve line break after headers issue #319 #321

Fix to preserve line break after headers issue #319 #321

BernieWhite commented Dec 4, 2017

BernieWhite commented Dec 4, 2017

vors commented Dec 5, 2017

vors left a comment

vors Dec 6, 2017

BernieWhite Dec 6, 2017

vors Dec 6, 2017

BernieWhite Dec 6, 2017

BernieWhite Dec 7, 2017

vors Dec 6, 2017

BernieWhite Dec 6, 2017

BernieWhite Dec 6, 2017

vors Dec 6, 2017

BernieWhite Dec 6, 2017

BernieWhite Dec 7, 2017

vors Dec 6, 2017

BernieWhite Dec 6, 2017

BernieWhite Dec 7, 2017

vors Dec 6, 2017

BernieWhite Dec 7, 2017

vors Dec 6, 2017

BernieWhite Dec 7, 2017

vors Dec 6, 2017

vors Dec 6, 2017

BernieWhite Dec 7, 2017

vors Dec 6, 2017

vors commented Dec 6, 2017

BernieWhite commented Dec 7, 2017

vors commented Dec 7, 2017

BernieWhite commented Dec 7, 2017

vors left a comment

vors Dec 8, 2017

vors Dec 8, 2017

vors Dec 8, 2017

Fix to preserve line break after headers issue #319 #321

Fix to preserve line break after headers issue #319 #321

Conversation

BernieWhite commented Dec 4, 2017

BernieWhite commented Dec 4, 2017

vors commented Dec 5, 2017

vors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vors commented Dec 6, 2017

BernieWhite commented Dec 7, 2017

vors commented Dec 7, 2017

BernieWhite commented Dec 7, 2017

vors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment