Add support for passing files and markdown directly to Show-Markdown
#7354
Conversation
SteveL-MSFT
requested review from
JamesWTruher,
PaulHigin and
dantraMSFT
as code owners
iSazonov
left a comment
iSazonov
left a comment
There was a problem hiding this comment.
LGTM with one minor comment.
There was a problem hiding this comment.
I believe we can remove System.IO.
There was a problem hiding this comment.
The compiler complained that Path type here was confused with the Path parameter.
There was a problem hiding this comment.
Two more comments:
-
ConvertFrom-Markdown e:\powershell\readme.mdthrows error:A positional parameter cannot be found that accepts argument 'E:\PowerShell\README.md'. I think it should work. -
ConvertFrom-Markdown -Path E:\PowerShell\README.md | Show-Markdownfails with this error:Show-Markdown : The property VT100EncodedString of the given object is null or empty.So,ConvertFrom-Markdownby default only populates HTML, whileShow-Markdownby default show VT100. That feels very inconsistent to me.
Opened issue #7363
There was a problem hiding this comment.
This constructor will create a new Runspace. Please use PowerShell.Create(RunspaceMode.CurrentRunspace) instead, which will use a nested pipeline in the same Runsapce.
There was a problem hiding this comment.
Use is MarkdownInfo markdownInfo, and pass in markdownInfo instead.
There was a problem hiding this comment.
Please make it throw an InvalidOperationException. The assertion is not that useful with our current building/testing.
There was a problem hiding this comment.
You can use Invoke<MarkdownInfo>(), so the output will be MarkdownInfo collection and we can avoid the type comparison in ProcessMarkdownInfo.
There was a problem hiding this comment.
If you address the comments above, this check will no longer be needed.
There was a problem hiding this comment.
This error message won't be needed if you address the above comments.
|
@daxian-dbw I think the |
There was a problem hiding this comment.
Nitpick. You don't need to call Stop(). Dispose() will do this for you.
There was a problem hiding this comment.
ConvertMarkdownStrings.InvalidInputObjectType
I guess this resource string can be removed now.
There was a problem hiding this comment.
That string is still used by ConvertFrom-Markdown and Set-MarkdownOption cmdlets
SteveL-MSFT
force-pushed
the
show-markdown-path
branch
from
72a8800 to
1b467d0
Compare
|
@dantraMSFT CI is failing on macOS oslog tests. I don't see any thing related to the changes I've made. Can you take a look? |
SteveL-MSFT
force-pushed
the
show-markdown-path
branch
from
6b9445e to
9f2811e
Compare
There was a problem hiding this comment.
The message should come from a resource file.
5 participants
PR Summary
Previously you had to use
ConvertFrom-Markdownand pipe that output toShow-Markdownmaking it cumbersome to use interactively. Added-Pathand-LiteralPathparameters as well as allowing markdown text to passed directly as-InputObject(or via pipeline) toShow-Markdown.Internally,
Show-MarkdownusesConvertFrom-Markdownto handle these new capabilities. Removed the use ofOut-Defaultwhich only passed the resulting vt100 text to the console and instead using WriteObject() so that the output can be redirected or captured by scripts or further in the pipeline. The testhook is still needed when-UseBrowseris specified.Note I had to fully qualify
PowerShellandPathtypes due to conflict withPathparameter andPowerShellnamespace.Fix #7338
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests