-
Notifications
You must be signed in to change notification settings - Fork 14
Add Microsoft.PowerShell.PlatyPS documentation #150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Microsoft.PowerShell.PlatyPS documentation #150
Conversation
Well, I heard about this promise. 🙃 Thank you very much for your contribution here. This is huge PR, so give us some time to review it. I briefly reviewed it and noticed a few flaws, hopefully small issues only. |
Gehe, I can recognize and no worries. Just let me know, I'm curious about the flaws if it was coming from the new PS module. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the new version of documentation; it contains many improved features, like:
- Aliases
- Keeps code-formatted texts
- Examples have PowerShell colour schema
-ProgressAction
common parameter moved to "CommonParameters" section
Major issues
- Previously we had generated index page (readme.md) with list of all functions and links to specific page. This disappers now.
Get-FabricWorkspaceUser
: INPUTS contains one item (System.Object) without description ({{ Fill in the Description }})Get-FabricDatasetRefreshes
- the same as above- Some examples/description have been broken now, for instance:
a) Wrong examples 1 and 3 forGet-FabricSQLDatabase
b) Incorrect format for Example 5 inGet-FabricWorkspaceUser
c) Invoke-FabricKQLCommand - Example 2, 3
d)Get-FabricSQLDatabase
- Example 3
e)Get-FabricDatasetRefreshes
- Example 1 - code section is not declared as code (```)
f) ... please check rest of the function with similar issues - Can we generate all files in the same folder as previosuly (Documentation)?
Minor issue (could be fixed later if possible):
- RELATED LINKS contains static placeholder "{{ Fill in the related links here }}"
You want me to patch these up Kamil :)? |
ahhh... yes. If you can and have the capacity, please do. You know the tool better than anyone of us, I reckon. |
Thanks a lot for taking the time @NowinskiK. I had created a script to loop through all files now, because clearly, when the SYNOPSIS is outside the function, nothing happens. That's why I re-inserted everything inside the functions now, which should have fixed most of the examples. Regarding point 1, I have now added an additional command in the mix: The Point 5 isn't a best practice to do. I can recognize that, if you've already specified other external tools to look to this folder, I would keep it "as-is". Otherwise, especially with localization, docs is the directory for many open-source PowerShell projects. That said, everyone has it's preferences of course. Lastly regarding the code-fencing. I opted to use this one, as it helps users on the docs itself to easily copy the command and try them out themselves. It comes down to a behaviour thingy. When new functions get introduced, I guess providing instructions what the way of documentating the functions should look like, helps to get it going. Of course, I can still loop through all the Markdown files to see were the current examples are different. Note We can always opt to use the |
Side note on the confirm and whatif, Aditya just created a PR: PowerShell/platyPS#768 (not released yet) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your hard work here @Gijsreyn.
Almost all good now, just few minor issues I managed to spot so far (I did not review all the files yet).
source/Public/Deployment Pipeline/Start-FabricDeploymentPipelineStage.ps1
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, all files reviewed now. I added few more comments.
Please check results of tests.
source/Public/Workspace/Get-FabricWorkspaceDatasetRefreshes.ps1
Outdated
Show resolved
Hide resolved
Hey @NowinskiK. Legend! I can imagine it was a little exhausting to validate, but I really appreciated it. I checked all files and removed the line breaks. Regarding the doc task itself, I have moved it to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last loop I hope.
source/Public/Deployment Pipeline/Start-FabricDeploymentPipelineStage.ps1
Outdated
Show resolved
Hide resolved
source/Public/Workspace/Get-FabricWorkspaceDatasetRefreshes.ps1
Outdated
Show resolved
Hide resolved
source/Public/Workspace/Get-FabricWorkspaceDatasetRefreshes.ps1
Outdated
Show resolved
Hide resolved
Just seeing through community call: https://www.powershellgallery.com/packages/Microsoft.PowerShell.PlatyPS/1.0.0-rc1. That would mean the Confirm and Whatif can be removed. Want me to push that still? |
Yes, please do. |
Cool, just added the .rc1. Keep in mind when working with scoped variables that underlying tests might fail. I have modified the name now. The test should be resolved. Please give it a go if you have time :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
We are good now. Thank you for your hard work, @Gijsreyn ! You rock! |
…sreyn/FabricTools into microsoft-powershell-platyps
And the same to you! I didn't make it easy for you, and I know it was a big PR. Thanks for all the checking. |
Pull Request
Never make a bearded man @SQLDBAWithABeard and a soaking-wet @jpomfret from bicycling a promise on a conference. Just kidding, it was on my TODO list.
Pull Request (PR) description
This pull request adds
Microsoft.PowerShell.PlatyPS
documentation, including a task to generate it. I saw the pull request coming in from @NowinskiK earlier, and I definitely didn't want to step on anyone's toes here. Instead of using the old PlatyPS module to generate documentation, Microsoft.PowerShell.PlatyPS is about to hit GA (as per Jason during the last PS community call).For an extensive comparison between the two, see the comment by Johan on: gaelcolas/Sampler#501
The task is also added to the
build.yaml
file, consistently producing up-to-date documentation.Note
(Nearly) all examples are updated with a PowerShell code block fenced
Task list
build.ps1 -ResolveDependency -Tasks build, test
).