Fix formatting, vt100 support, help uri and positional parameter issues for Markdown cmdlets by adityapatwardhan · Pull Request #7329 · PowerShell/PowerShell

adityapatwardhan · 2018-07-21T00:45:31Z

PR Summary

Formatting changes moved from types file to formatting file.
Add logic to check VT100 support before adding VT100 escape sequences.
Add online help uris.
Add positional parameters for Convert-FromMarkdown and Set-MarkdownOption

PR Checklist

…meter issues for Markdown cmdlets * Formatting changes moved from types file to formatting file. * Add logic to check VT100 support before adding VT100 escape sequences. * Add online help uris. * Add positional parameters for Convert-FromMarkdown and Set-MarkdownOption

SteveL-MSFT · 2018-07-21T15:39:55Z

src/Microsoft.PowerShell.MarkdownRender/VT100EscapeSequences.cs

There's a lot of similar code, perhaps have a helper that does the EnableVT100Encoding check and concats or just returns the string?

Fixed for header types. For others the code is slightly different for each so kept it as is.

iSazonov · 2018-07-23T06:06:28Z

Usually we use

[Alias("PSPath", "LP")]

for LiteralPath parameters.

iSazonov

Leave a comment

iSazonov · 2018-07-23T06:10:43Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/ConvertFromMarkdownCommand.cs

What do you think about "MarkdownOptionInfo" -> "PSMarkdownOptionInfo"?

I prefer MarkdownOptionInfo since the cmdlet is Get-MarkdownOption. @daxian-dbw your thoughts?

I think for any variables that are part of PowerShell itself, it should have the PS prefix to avoid name collision. So this should be PSMarkdownOptionInfo. cc @joeyaiello

The variable MarkdownOptionInfo should be saved in the module scope of the Utility module, instead of the current scope of the current session state. Doing the latter will make the persistence of markdown option unreliable/inconsistent.

And, did you consider other options to persist the markdown option? I don't have an alternative off the top of my mind, but would like to check if you considered anything else.

@SteveL-MSFT if we agree it should be moved to the module state, does it matter?

As for persistence, I think it's okay to tell people to put this stuff in their profile until we hear why that's not good enough (seems to work just fine for PSReadline).

@daxian-dbw The committee feedback was to have it in session state so it is valid throughout the session. The preferences can be set in profile for persistence.

I'm OK to have it in session state, but it should be the module session state of Microsoft.PowerShell.Utility instead of the currently active session state, which could be the global session state or another module's session state. And the scope to save the variable should be the module scope, not the current scope.

For example, if I'm running Set-MarkdownOptions in a function from another module, then the variable will be saved in the function scope under the session state of that module, which will be lost once the function finishes execution.

rkeithhill · 2018-07-23T15:57:06Z

👍 for positional parameter support. When ConvertFrom-Markdown .\README.md generated an error, I was bummed.

adityapatwardhan · 2018-07-23T17:12:17Z

@SteveL-MSFT please have another look.

PaulHigin · 2018-07-23T18:08:57Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/ConvertFromMarkdownCommand.cs

this.Host can be null if the cmdlet is invoked in a runspace without a PSHost.

SteveL-MSFT · 2018-07-23T19:08:54Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/ConvertFromMarkdownCommand.cs

I think for any variables that are part of PowerShell itself, it should have the PS prefix to avoid name collision. So this should be PSMarkdownOptionInfo. cc @joeyaiello

SteveL-MSFT · 2018-07-24T20:08:37Z

On macOS using the Terminal app, it doesn't seem to display the VT100 inverted sequence so the text becomes unreadable since the color is supposed to be black on inverted background but just appears as black. A basic web search doesn't find anything on how to support this so we may need to special case macOS to use some other colors.

SteveL-MSFT · 2018-07-24T23:49:47Z

I take back part of what I said. Inverse works for Headers like

# header

But this only renders as black text on black background

`text`

adityapatwardhan · 2018-07-25T17:52:01Z

@PaulHigin @SteveL-MSFT @daxian-dbw Please have another look.

daxian-dbw · 2018-07-25T18:10:44Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/ConvertFromMarkdownCommand.cs

Same comment as in #7329 (comment)

SteveL-MSFT · 2018-07-25T18:13:57Z

src/Microsoft.PowerShell.MarkdownRender/VT100EscapeSequences.cs

        private const string Header6Dark = "[4;97m";
        private const string CodeDark = "[48;2;155;155;155;38;2;30;30;30m";
+
+        private const string CodeMacOS = "[107;95m";


Should add a comment why we are special casing macOS

Added comment.

PaulHigin · 2018-07-25T20:07:08Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/ConvertFromMarkdownCommand.cs

this.Host.UI.SupportsVirtualTerminal is not needed twice here. But it seems like the logic should be:

mdOption.EnableVT100Encoding = mdOption.EnableVT100Encoding && (this.Host != null) && (this.Host.UI.SupportsVirtualTerminal); If there is no host I assume EnableVT100Encoding option should be disabled?

@PaulHigin improved it a bit by using nullable bool.

adityapatwardhan · 2018-07-26T18:32:52Z

@PaulHigin @SteveL-MSFT @daxian-dbw Please have a look.

PaulHigin · 2018-07-26T18:42:57Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/ConvertFromMarkdownCommand.cs

Nit. I think you can just do if (supportsVT100 != true) here.

PaulHigin

LGTM

SteveL-MSFT · 2018-07-26T19:57:48Z

@adityapatwardhan can you review the CodeFactor issues?

adityapatwardhan · 2018-07-26T23:24:10Z

@SteveL-MSFT I have fixed a few CodeFactor issues. Others are either following a pattern which already exists or a rule we want to disable.

@daxian-dbw Can you have another look?

daxian-dbw

One more comment. Otherwise look good to me.

daxian-dbw · 2018-07-27T00:11:53Z

test/powershell/Modules/Microsoft.PowerShell.Utility/MarkdownCmdlets.Tests.ps1

+        It "Verify PSMarkdownOptionInfo is defined in module scope" {
+
+            $mod = Get-Module Microsoft.PowerShell.Utility
+            $options =  & $mod { $PSMarkdownOptionInfo }


Better add one check for $PSMarkdownOptionInfo -eq $null before this line, to make sure that the variable is not leaked into the global scope or current scope.

iSazonov · 2018-07-27T04:18:51Z

@daxian-dbw Is your #7329 (comment) closed?

adityapatwardhan · 2018-07-27T17:27:12Z

@daxian-dbw Ready to merge?

daxian-dbw · 2018-07-28T00:20:26Z

@iSazonov Yes, it has been changed to use the module scope to save the variable.

adityapatwardhan assigned daxian-dbw Jul 21, 2018

adityapatwardhan requested a review from daxian-dbw July 21, 2018 00:45

adityapatwardhan requested review from BrucePay, JamesWTruher, PaulHigin and dantraMSFT as code owners July 21, 2018 00:45

adityapatwardhan added 2 commits July 20, 2018 17:47

[Feature] Remove extra line in CSV file

292ed24

[Feature] Fix CodeFactor issue

bad774c

SteveL-MSFT requested changes Jul 21, 2018

View reviewed changes

iSazonov reviewed Jul 23, 2018

View reviewed changes

[Feature] Address code review comments

163393c

PaulHigin suggested changes Jul 23, 2018

View reviewed changes

SteveL-MSFT requested changes Jul 23, 2018

View reviewed changes

[Feature] Address code review comments

60e8272

adityapatwardhan requested review from TravisEz13 and anmenaga as code owners July 25, 2018 17:51

daxian-dbw requested changes Jul 25, 2018

View reviewed changes

daxian-dbw added the WG-Cmdlets-Utility cmdlets in the Microsoft.PowerShell.Utility module label Jul 25, 2018

SteveL-MSFT requested changes Jul 25, 2018

View reviewed changes

PaulHigin suggested changes Jul 25, 2018

View reviewed changes

adityapatwardhan added 2 commits July 26, 2018 11:06

[Feature] Move PSMarkdownOptionInfo variable to module scope

e1101ed

[Feature] Add MacOS comment and improve host check

5dcbb8e

PaulHigin reviewed Jul 26, 2018

View reviewed changes

PaulHigin approved these changes Jul 26, 2018

View reviewed changes

[Feature] Fix tests of macOS and addres comments

60cfa7c

SteveL-MSFT approved these changes Jul 26, 2018

View reviewed changes

[Feature] Fix some CodeFactor issues and macOS test failure

e1d3426

daxian-dbw requested changes Jul 27, 2018

View reviewed changes

SteveL-MSFT approved these changes Jul 27, 2018

View reviewed changes

[Feature] Added check for PSMarkdownOptionInfo in current scope

cf2c028

daxian-dbw approved these changes Jul 27, 2018

View reviewed changes

TravisEz13 approved these changes Jul 27, 2018

View reviewed changes

daxian-dbw merged commit 4e7c2ed into PowerShell:master Jul 28, 2018

adityapatwardhan deleted the FixMarkdownCmdletIssues branch July 28, 2018 15:45

This was referenced Aug 6, 2018

NullReferenceException in ConvertFrom-Markdown -AsVT100EncodedString with empty alt text #7453

Closed

Show-Markdown unusable on a dark terminal theme #7454

Closed

adityapatwardhan mentioned this pull request Aug 13, 2018

'ConvertFrom-Markdown' should take positional argument and 'Show-Markdown' should have consistent default as 'ConvertFrom-Markdown' #7363

Closed

Conversation

adityapatwardhan commented Jul 21, 2018

PR Summary

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Jul 23, 2018

Uh oh!

iSazonov 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!

rkeithhill commented Jul 23, 2018

Uh oh!

adityapatwardhan commented Jul 23, 2018

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!

SteveL-MSFT commented Jul 24, 2018

Uh oh!

SteveL-MSFT commented Jul 24, 2018

Uh oh!

adityapatwardhan commented Jul 25, 2018

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!

adityapatwardhan commented Jul 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PaulHigin left a comment

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT commented Jul 26, 2018

Uh oh!

adityapatwardhan commented Jul 26, 2018

Uh oh!

daxian-dbw 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!

iSazonov commented Jul 27, 2018

Uh oh!

adityapatwardhan commented Jul 27, 2018

Uh oh!

daxian-dbw commented Jul 28, 2018

Uh oh!