import draft #69
import draft #69
Conversation
There was a problem hiding this comment.
Heya, thanks for that command :)
See comments for specific feedback.
Other than those:
Display Style
Have you considered writing the actual display text using Write-PSFHostColor rather than Write-PSFMessage?
That would allow you to avoid all the message decorations (Timestamp and command name)
After all, those aren't really messages you are writing - it's console display, designed to visualize information to the user.
| - ShowParameters | ||
|
|
||
| .PARAMETER IncludeHelp | ||
| Switch to instruct the cmdlet / function to output a simple guide with the colors in it |
There was a problem hiding this comment.
I think this parameter should be named -Legend instead, as that's what it really enables
Also the description is slightly confusing:
- The switch part is redundant (the fact that it is a switch is shown as the type behind the name when calling help)
- cmdlet / function differentiation is not a subject needed
- Referring to the command you are explaining is counter-intuitive (we already are explaining this command, so what is it referring to?)
Instead something like:
Include a legend explaining the color mapping / meaning
Would be very clear and concise about its purpose
| Switch to instruct the cmdlet / function to output a simple guide with the colors in it | ||
|
|
||
| .EXAMPLE | ||
| PS C:\> Show-PSMDSyntax -CommandText 'Import-D365Bacpac -ImportModeTier2 -SqlUser "sqladmin" -SqlPwd "XyzXyz" -BacpacFile2 "C:\temp\uat.bacpac"' -Mode "Validate" |
There was a problem hiding this comment.
General note on examples:
Those should be executable without any extra module dependency, if practicable (something I myself have sometimes gotten wrong, but fix where I touch it). I know it's a copy from a command in your own module (where no extra dependency is added obviously, but would appreciate the update none-the-less
| This will display all the parameter sets and their individual parameters. | ||
|
|
||
| .NOTES | ||
| Author: Mötz Jensen (@Splaxi) |
There was a problem hiding this comment.
Insufficient notes.
To properly credit your contribution, please also include a link to twitter and your github repository where you copied it from.
I totally want people to find your other cool stuff, if they are interested enough to look at the notes :)
|
|
||
| [Parameter(Mandatory = $true, Position = 2)] | ||
| [ValidateSet('Validate', 'ShowParameters')] | ||
| [string] $Mode, |
There was a problem hiding this comment.
Why is this parameter mandatory?
Wouldn't a default value of 'Validate' be more convenient for the user?
Also: Why include modes at all?
I have yet to see a meaningful difference between the modes.
There was a problem hiding this comment.
I agree on the default value, that it should be validate.
The main reason for the 2 modes is maybe a bit vague, but the validate shows asterisk all around the output. From a user perspective I thought that it might confuse people the first few times, until the understood the function. Let's visit this when we have nailed the rest.
| return | ||
| } | ||
| } | ||
| else { |
There was a problem hiding this comment.
Terminating loop conditions should not be placed in a trailing else block - move above the current if position, save yourself a nested braces level and make the code more readable.
| } | ||
| } | ||
| else { | ||
| Write-PSFMessage -Level Host -Message "The function was unable to get the help of the command. Make sure that the command name is valid and try again." |
There was a problem hiding this comment.
Terminating loop conditions should not be placed in a trailing else block - move above the current if position, save yourself a nested braces level and make the code more readable.
| "ShowParameters" { | ||
| foreach ($parmSet in (Get-Command $commandName).ParameterSets) { | ||
| # (Get-Command $commandName).ParameterSets | ForEach-Object { | ||
| $null = $sb = New-Object System.Text.StringBuilder |
There was a problem hiding this comment.
What is the point in this null-assignment?
| } | ||
|
|
||
| foreach ($parmSet in (Get-Command $commandName).ParameterSets) { | ||
| $null = $sb = New-Object System.Text.StringBuilder |
There was a problem hiding this comment.
Superfluous null-assignment
|
|
||
| if ($parmFoundInCommandText) { | ||
| $color = "$colorFoundAsterisk" | ||
| $null = $sb.Append("<c='$color'>* </c>") |
There was a problem hiding this comment.
This can be compacted to:
$null = $sb.Append("<c='$colorFoundAsterisk'>* </c>")
|
Another thing I noted: Show-PSMDSyntax -CommandText 'Get-Help -Name foo -Examples -Parameter bar' -Mode ValidateThis will happily show all parametersets. It may be a good idea to warn about impossible parameter sets. Maybe even separate the parameter sets as they are displayed between parametersets still possible and parametersets no longer possible (the impossible ones displaying the parameters that cannot be fit into them in a special color) |
|
I hadn't been working with Write-PSFHostColor earlier and you were away, so I just started with what i had of available tools. That should be fixed now. Regarding the parametersets and impossible ones - that was actually not part of the plan, but I like the idea. That might just require a bit more understanding from my side. I'll give it a look. |
…/PSModuleDevelopment into implement-show-psmdsyntax
|
Did most of the updates. Let us see how it goes. Then we could discuss more about the invalid parametersets. |
|
Thanks for all the work on this, merged it into dev, will be in the next release :) |
No description provided.