Adding New-YamlHelp cmdlet as well as adding module data to cmdlet pages #277

Sampy · 2017-06-12T22:42:05Z

This code is to support using platyPS as part of the process for getting the documentation of Microsoft Powershell modules up on Docs.Microsoft.com.

There are two main features:

Create a new New-YamlHelp cmdlet which, similar to New-ExternalHelp, reads platyPS Markdown and converts it into a set of YAML files. These files are much easier for external systems to parse and allow platyPS to convert the easy to edit Markdown files into easy to process data structures rather than having external tools attempt to parse the Markdown schema themselves.
Add the "Module Name" metadata to each cmdlet Markdown file. This makes it easier for external tools to know the source of each cmdlet which is useful when rendering a cmdlet document page.

The biggest changes include the new cmdlet, a new renderer and supporting models, and the addition of YamlDotNet as a dependency to handle the Yaml processing. Unit tests have been added for the new renderer.

…rkdown pages

msftclas · 2017-06-12T22:42:10Z

@Sampy,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

vors · 2017-06-12T23:24:00Z

Thank you @Sampy ! Love that you are contributing to platyPS!

Add the "Module Name" metadata to each cmdlet Markdown file.

There is a little bit overlap with ModulePage concept which @jongeller designed.
Example https://github.com/PowerShell/PowerShell-Docs/blob/staging/reference/6/Microsoft.PowerShell.Core/Microsoft.PowerShell.Core.md

I'd like to better understand the scenario that you are envision. Can you, elaborate, please?

Sampy · 2017-06-12T23:29:51Z

Happy to explain! For reference, here is a sample of the page we generate for a cmdlet today: Add-AzureAnalysisServicesAccount

The idea is that when we take the structure data about a cmdlet to generate a page, we'd like to, if possible, read all the information we need to make a page from a single file. Under the name of the cmdlet, we put a link to the module page. By adding the module name to the Markdown file, it's easy to use this piece of metadata to generate the Yaml file in call cases (passing a full directory as well as a single .md file to New-YamlHelp).

vors

Files under src/Markdown.MAML/Model/YAML looks very similar to files under src/Markdown.MAML/Model/MAML. Does it worth to have 2 hierarchies of very similar classes? Can you simply re-use existing ones maybe?

vors · 2017-06-15T03:54:53Z

src/platyPS/platyPS.psd1

@@ -70,6 +70,7 @@ FunctionsToExport = @(
    'New-MarkdownHelp', 
    'Get-MarkdownMetadata',
    'New-ExternalHelp', 
+	'New-YamlHelp', 


Use spaces, not tabs

vors · 2017-06-15T03:56:49Z

src/platyPS/platyPS.psm1

@@ -456,6 +459,7 @@ function Update-MarkdownHelp
            $metadata = Get-MarkdownMetadata $filePath
            $metadata["external help file"] = GetHelpFileName $command
            $reflectionModel = GetMamlObject -Cmdlet $name
+			$metadata[$script:MODULE_PAGE_MODULE_NAME] = $reflectionModel.ModuleName


4 spaces for indentation, please

vors · 2017-06-15T03:57:30Z

src/platyPS/platyPS.psm1

@@ -200,10 +200,13 @@ function New-MarkdownHelp

                        $helpFileName = GetHelpFileName (Get-Command @a)    
                    }
+
+					Write-Verbose "Maml things module is: $($mamlObject.ModuleName)"


same : 4 spaces

vors · 2017-06-15T04:01:16Z

src/Markdown.MAML/packages.config

+<?xml version="1.0" encoding="utf-8"?>
+<packages>
+  <package id="YamlDotNet" version="4.2.1" targetFramework="net45" />
+</packages>


This is nuget config, right?
Can you, consolidate it with .nuget/* files?
I don't know much about modern way of managing dependencies with nuget. We have this checked-in nuget.exe which doesn't seem like a good idea.
Maybe we can use adding new dependency as an oportunity to streamline this?

Each project needs it's own packages.config for the things it depends on (just like the test project has the xunit libraries). You can get rid of the .nuget/* files if your project is running on .Net Core in VS 2017 since NuGet is built in to the new tool chain but this is still the proper way to do things for .Net Framework projects

vors · 2017-06-15T04:03:42Z

src/Markdown.MAML/Model/MAML/MamlCommand.cs

@@ -31,6 +31,8 @@ public List<MamlParameter> Parameters

        public bool SupportCommonParameters { get; set; }

+        public string ModuleName { get; set; }


There are cases when it's null.
I.e. when the function is dynamically defined, we should handle this cases gracefully.

From looking at the code where we read this property, null is an acceptable value.

vors · 2017-06-15T04:05:57Z

src/platyPS/platyPS.psm1

+			}
+			$mamlModel = $mamlModels[0]
+			$markdownMetadata = Get-MarkdownMetadata -Path $MarkdownFile.FullName
+			$mamlModel.ModuleName = $markdownMetadata[$script:MODULE_PAGE_MODULE_NAME]


This is clunky: ModuleName becomes this only mutable part of $mamlModel that we have to set from PS.
It would be cleaner to pass it as a parameter to MamlModelTransformer.

UPD: Actually, it could be even more clunkier, because of the burden of passing a parameter thru all the function... Not sure, maybe it's fine as is. If you think it's fine as is, please add a comment about it.

Comment explaining why added. I think a long term fix could include having this piece of data read by the markdown parser but it skips the Yaml header at the moment.

vors · 2017-06-15T04:06:43Z

src/platyPS/platyPS.psm1

@@ -599,6 +603,70 @@ function New-MarkdownAboutHelp
    }
 }

+function New-YamlHelp


Please include markdown docs for the new function in the PR

vors · 2017-06-15T04:07:52Z

src/platyPS/platyPS.psm1

+	end 
+	{
+		$MarkdownFiles | ForEach-Object {
+            Write-Verbose "[New-ExternalHelp] Input markdown file $_"


Fix verbose messages to use the right cmdlet name

vors · 2017-06-15T04:08:50Z

src/platyPS/platyPS.psm1

+		foreach($markdownFile in $MarkdownFiles)
+		{
+			$mamlModels = GetMamlModelImpl $markdownFile.FullName -Encoding $Encoding
+			if($mamlModels.Count -ne 1)


This if is not needed anymore actually, you can remove it completely

I've updated this block of code to handle a scenario where GetMamlModelImpl returns me more than one cmdlet

vors · 2017-06-15T04:10:53Z

src/platyPS/platyPS.psm1

+        [string[]]$Path,
+
+        [Parameter(Mandatory=$true)]
+        [string]$OutputPath,


If you want ot use $OutputPath and not $OutputFolder, please add handling for both paths and folders cases. For example, see how New-ExternalHelp allows to pass both dir and *.xml file as $OutputPath parameter

I've updated this to use OutputFolder as the parameter name since that is the convention

vors · 2017-06-15T04:13:22Z

Can you, please, include a sample of generated yaml?

vors · 2017-06-15T04:47:59Z

@Sampy you would likely need to get a Microsoft legal team approval to include a new dependency into this OSS project (the YamlDotNet dll). It's an internal procedure, @joeyaiello or @SteveL-MSFT should be able to work with you on that.

Sampy · 2017-06-16T19:02:45Z

I will check and see what sort of legal approval we need to distribute YamlDotNet and figure out what to do there.

Would you like me to check a sample Yaml file into the repo somewhere? I don't see a really good place for it but I'm happy to include it where ever makes sense.

Sampy · 2017-06-16T19:10:32Z

As far as re-using the MAML model classes, the big reason I didn't want to reuse them was to allow these two different models to grow independently. As time goes on, more data may be added to either of the two structures that the other may or may not need. By keeping the YAML model isolated, I was thinking that we could keep it as clean as possible as to which data is needed in each case.

joeyaiello · 2017-06-16T19:42:38Z

@vors Thanks for pointing that out, I'm getting all the legal approval taken care of from our side. I'll ping back here when that's done (hopefully super soon).

vors

Love the documentation you wrote!

Include cmdlet pester tests for New-YamlHelp to the platyPS.Tests.ps1

^^^ sorry, missed that the first time

vors · 2017-06-17T03:32:57Z

src/platyPS/platyPS.psm1

+
+                $yaml = [Markdown.MAML.Renderer.YamlRenderer]::MamlModelToString($mamlModel)
+                $outputFilePath = Join-Path $OutputFolder ($mamlModel.Name + ".yml")
+                Write-Verbose "Writing external help to $outputFilePath"


"Writing yaml help..."

vors · 2017-06-17T03:36:27Z

@Sampy no need to commit it to the repo, you can create a gist and link it here in the PR. I just want to take a look at it.

Sampy · 2017-06-19T22:38:17Z

Here is the gist for a sample Yaml file: https://gist.github.com/Sampy/77ff4d6ddad5bb555b36d5f793f3d73b

vors

@Sampy please remove yaml files and folder
@joeyaiello this PR is on hold until the legal team approval.

LGTM

vors · 2017-06-21T19:59:31Z

test/Pester/PlatyPs.Tests.ps1

+        $yamlModel.OptionalParameters[1].Name | Should Be 'Force'
+    }
+
+    It 'throws for OutputFolder that is a file'{


nit: space before {

vors · 2017-06-21T20:00:44Z

yaml/Get-HelpPreview.yml

@@ -0,0 +1,78 @@
+examples:


No need to include this yaml folder into PR. We only should keep the sources under the source control. These files are purely the artifacts from the markdown, similar to maml xmls.

vors · 2017-06-21T20:06:16Z

Assigning to Joey for follow-up

Adding New-YamlHelp cmdlet as well as adding module data to cmdlet ma…

ee2b039

…rkdown pages

msftclas added the cla-not-required label Jun 12, 2017

vors suggested changes Jun 15, 2017

View reviewed changes

Sampy added 3 commits June 16, 2017 07:40

Responding to code review feedback (part 1)

f0895a8

More code review feedback responses

04e0c24

Merge

9994d23

vors suggested changes Jun 17, 2017

View reviewed changes

vors self-assigned this Jun 18, 2017

Merge

a9fe9f2

Adding platyPS tests for New-YamlHelp

3977e18

vors suggested changes Jun 21, 2017

View reviewed changes

vors assigned joeyaiello and unassigned vors Jun 21, 2017

vors approved these changes Jun 21, 2017

View reviewed changes

Removing yaml folder

719bb37

SteveL-MSFT assigned adityapatwardhan Aug 4, 2017

adityapatwardhan merged commit b89efd8 into PowerShell:master Aug 4, 2017

		@@ -31,6 +31,8 @@ public List<MamlParameter> Parameters

		public bool SupportCommonParameters { get; set; }

		public string ModuleName { get; set; }

Adding New-YamlHelp cmdlet as well as adding module data to cmdlet pages #277

Adding New-YamlHelp cmdlet as well as adding module data to cmdlet pages #277

Uh oh!

Conversation

Sampy commented Jun 12, 2017

Uh oh!

msftclas commented Jun 12, 2017

Uh oh!

vors commented Jun 12, 2017

Uh oh!

Sampy commented Jun 12, 2017

Uh oh!

vors left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vors Jun 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vors commented Jun 15, 2017

Uh oh!

vors commented Jun 15, 2017

Uh oh!

Sampy commented Jun 16, 2017

Uh oh!

Sampy commented Jun 16, 2017

Uh oh!

joeyaiello commented Jun 16, 2017

Uh oh!

vors left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vors commented Jun 17, 2017

Uh oh!

Sampy commented Jun 19, 2017

Uh oh!

vors left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

vors Jun 15, 2017 •

edited

Loading