Added Meta, Charset, and Transitional parameters to ConvertTo-HTML by ergo3114 · Pull Request #4184 · PowerShell/PowerShell

ergo3114 · 2017-07-04T18:48:02Z

Related #3267

Added the Meta, Chartset, and Transitional parameters to ConvertTo-HTML

Meta Allows the user to insert <meta> tag information such as refresh, author, and keywords
Charset Sets the html encoding of the page, denoted by the <charset> html tag
Transitional A switch parameter that allows for the insertion of XHTML Transitional DTD DOCTYPE, the default being XHTML Strict DTD

Edit: Documentation can be found here

…false

…nd transitional DOCTYPEs - Updated Meta parameter to handle http-equiv properties

msftclas · 2017-07-04T18:48:06Z

@ergo3114,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

TravisEz13 · 2017-07-11T19:31:03Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/convert-HTML.cs

+                    {
+                        if(s != "content-type" && s != "default-style" && s !="refresh")
+                        {
+                            WriteObject("<meta name=\"" + s + "\" content=\"" + _meta[s] + "\">");


should we encode the key and value?

What would encoding gain us here? I am not familiar with that.

TravisEz13 · 2017-07-11T19:32:00Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/convert-HTML.cs

                WriteObject("<head>");
+                if(_charsetSpecified)
+                {
+                    WriteObject("<meta charset=\"" + _charset + "\">");


Should probably validate or encode the character set.

Validation would be great. I don't see too many changes in the charset list but it would be great if the validation were dynamic pulling from w3schools or something. I was trying to find documentation on what the browser does when a non standard charset is specified; such as "foo". I believe it analyzes the html and determines the best or just defaults to UTF-8 for HTML5.

I'm sure there are illegal characters. Even a validation that validates the user hasn't specified something illegal would be better than nothing.

I will put something in and resubmit it.

TravisEz13 · 2017-07-11T19:33:06Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/convert-HTML.cs

+                            WriteObject("<meta name=\"" + s + "\" content=\"" + _meta[s] + "\">");
+                        }
+                        else {
+                            WriteObject("<meta http-equiv=\"" + s + "\" content=\"" + _meta[s] + "\">");


Should we allow these or should it be another parameter like charset

It looks like you can validate the meta tag. w3schools shows the schema for the meta tag.

Could you use ValidatePattern or ValidateScript to implement it? https://blogs.technet.microsoft.com/heyscriptingguy/2011/01/11/validate-powershell-parameters-before-running-the-script/

TravisEz13 · 2017-07-11T19:37:10Z

@JamesWTruher Can you review the character set parameter here?

TravisEz13

Thanks for the PR. I have a couple of comments. Please review and respond.

- Set the output of Charset to upper case if a lowercase value is given

…eta values

SteveL-MSFT · 2017-07-16T05:21:45Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/convert-HTML.cs

+            }
+            set
+            {
+                _meta = value;


I believe the typical pattern is to use InvocationInfo.BoundParameters. This would then allow you to use the automatic properties to simplify the code a bit

I was unable to get InvocationInfo.BoundParameters to work. I read over InvocationInfo.cs and ImplicitRemotingCommands.cs to see if I can make sense of it but I am at a loss here.

We can use an auto property and Meta == null to check if user assign a value.

That should be fine, too

@ergo3114 Please address this.

I see this as two ways to do the same thing. What are you looking to gain from setting a default value versus checking a flag? Just curious.

A quick search shows we already have inconsistency so I'm fine staying with this pattern

SteveL-MSFT · 2017-07-16T05:32:30Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/convert-HTML.cs

+                {
+                    foreach(string s in _meta.Keys)
+                    {
+                        if(s == "content-type" && s == "default-style" && s =="refresh")


Shouldn't this be ||? In any case, there's more http-equiv than this list. Perhaps we should just have a separate parameter for http-equiv meta tags and let the user put whatever they want in it? Also, I believe the names are case-insensitive so Content-Type is valid.

This has been changed.

SteveL-MSFT · 2017-07-16T06:15:57Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/convert-HTML.cs

+        /// Specifies the charset encoding for the HTML document
+        /// </summary>
+        [Parameter(ParameterSetName = "Page")]
+        [ValidateNotNullOrEmpty]


Technically, the allowed set of (legacy) encodings is quite large although HTML5 I believe has standardized on UTF-8. So perhaps this shouldn't have a ValidateSet?

I have removed to the ValidateSet.

SteveL-MSFT

Please add some tests

- Removed Validateset from Charset parameter to allow for legacy character sets - Added test for Meta, Charset, and Transitional property on ConvertTo-HTML command

….BoundParameters

iSazonov · 2017-07-17T08:19:32Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/convert-HTML.cs

+            }
+            set
+            {
+                _meta = value;


We can use an auto property and Meta == null to check if user assign a value.

iSazonov · 2017-07-17T08:23:35Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/convert-HTML.cs

+            }
+            set
+            {
+                _charset = value.ToUpper();


Why do we convert a charset to upper case?
It seems we can remove this and use auto property.

iSazonov · 2017-07-17T08:29:43Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/convert-HTML.cs

+            }
+            set
+            {
+                _transitional = value;


We can use an auto property for a switch parameter without getter/setter.

…meter - Removed private variable for transitional switch parameter - Removed toUpper() functin from charset

TravisEz13 · 2017-08-21T17:59:40Z

@ergo3114 Thanks for the update. It's helpful for us to know the PR is not abandoned.

ergo3114 · 2017-08-25T01:16:14Z

I believe I am back in the game with continuing this PR. I will work on items that were left unattended in the coming days.

- Set meta hashtable keys to lower before checking in switch statement - Updated test for the transitional parameter to check only the first line of the result - Updated test to be more specific on the error message that is returned when thrown - Added logic to prevent duplicate meta tags

dantraMSFT · 2017-08-31T17:58:17Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/convert-HTML.cs

+        /// </summary>
+        /// <returns></returns>
+        [Parameter(ParameterSetName = "Page")]
+        [ValidateNotNullOrEmpty]


I think you want { get; set; } here instead of a field.

SteveL-MSFT · 2017-08-31T19:08:41Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/convert-HTML.cs

+                                    break;
+                            }
+                            useditems.Add(s);
+                        }


seems like we should give a warning if we are dropping duplicates

I have changed it to a warning and decided to keep the meta tags that don't match the switch.

SteveL-MSFT · 2017-09-06T15:52:06Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/convert-HTML.cs

-                                    ThrowTerminatingError (new ErrorRecord(exc, "MetaPropertyNotFound", ErrorCategory.ParserError, null));
+                                    //Exception exc = new NotSupportedException(StringUtil.Format(ConvertHTMLStrings.MetaPropertyNotFound, s));
+                                    MshCommandRuntime mshCommandRuntime = this.CommandRuntime as MshCommandRuntime;
+                                    string Message = "Accepted meta properties are content-type, default-style, application-name, author, description, generator, keywords, x-ua-compatible, and viewport. The meta pair: " + s + " and " + _meta[s] + " may not function correctly.";


Add a new string in the resx file so that it can be localized

SteveL-MSFT · 2017-09-06T15:52:51Z

test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertTo-Html.Tests.ps1

-    It "Test ConvertTo-HTML meta with invalid properties should throw"{
-        { ($customObject | ConvertTo-HTML -Meta @{"authors"="John Doe";"keywords"="PowerShell,PSv6"}) -join $newLine } | Should Throw "authors is not a supported meta property. Accepted meta properties are content-type, default-style, application-name, author, description, generator, keywords, x-ua-compatible, and viewport."
+    It "Test ConvertTo-HTML meta with invalid properties should throw warning" {
+        ($customObject | ConvertTo-HTML -Meta @{"authors"="John Doe";"keywords"="PowerShell,PSv6"} 3>&1) -match "Accepted meta properties are" | Should Be $true


Since the message could be localized, you probably just need to check that the warning contains the Meta tags you used in the test

SteveL-MSFT · 2017-09-07T16:13:36Z

test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertTo-Html.Tests.ps1

+    }
+
+    It "Test ConvertTo-HTML meta with invalid properties should throw warning" {
+        ($customObject | ConvertTo-HTML -Meta @{"authors"="John Doe";"keywords"="PowerShell,PSv6"} 3>&1) -match "Accepted meta properties are" | Should Be $true


Since the warning message can be localized, you shouldn't check against the English string. Instead, check that the warning message contains the meta pair.

…output

…ters to ConvertTo-HTML (PowerShell#4184) (reverted from commit bbc180a)

That PR incorrectly included Pester changes. Most likely because that individual did not update his submodules.

ergo3114 added 5 commits June 27, 2017 20:51

- Added functionality to convert-HTML.cs to include a charset parameter

be4d32c

- Fixed issues on the charset parameter where flag was always set to …

883df45

…false

- Moved Meta tag into Head tag

3caab36

- Took away charset defaullt value

cdb3535

- Added Transitional switch to allow users to switch between strict a…

d0222c5

…nd transitional DOCTYPEs - Updated Meta parameter to handle http-equiv properties

msftclas added the cla-already-signed label Jul 4, 2017

ergo3114 mentioned this pull request Jul 4, 2017

Documentation of Meta, Charset and Transitional parameter for ConvertTo-HTML MicrosoftDocs/PowerShell-Docs#1432

Merged

8 tasks

daxian-dbw requested a review from SteveL-MSFT July 5, 2017 22:55

daxian-dbw assigned TravisEz13 Jul 5, 2017

TravisEz13 added the Documentation Needed in this repo Documentation is needed in this repo label Jul 11, 2017

TravisEz13 reviewed Jul 11, 2017

View reviewed changes

TravisEz13 requested a review from JamesWTruher July 11, 2017 19:36

TravisEz13 previously requested changes Jul 11, 2017

View reviewed changes

ergo3114 added 3 commits July 15, 2017 15:47

- Added validation to the Charset parameter

35b44c0

- Set the output of Charset to upper case if a lowercase value is given

- Put the Meta parameter output inside the HTML head tag

6b325b7

- Added filtering for the Meta parameter to only allow defined HTML m…

7d39a6c

…eta values

SteveL-MSFT reviewed Jul 16, 2017

View reviewed changes

SteveL-MSFT requested changes Jul 16, 2017

View reviewed changes

ergo3114 added 2 commits July 16, 2017 13:06

- Changed the condition for http-equiv from && to ||

ac28b95

- Removed Validateset from Charset parameter to allow for legacy character sets - Added test for Meta, Charset, and Transitional property on ConvertTo-HTML command

- Removed static declaration while attempting to using InvocationInfo…

3baca91

….BoundParameters

iSazonov reviewed Jul 17, 2017

View reviewed changes

ergo3114 added 3 commits July 22, 2017 17:16

- Removed defined get/set functions from the transitional switch para…

56a7db3

…meter - Removed private variable for transitional switch parameter - Removed toUpper() functin from charset

- Removed commented out testing

71168cc

Merge remote-tracking branch 'refs/remotes/PowerShell/master'

06ef847

ergo3114 requested review from PaulHigin and dantraMSFT as code owners August 27, 2017 17:19

ergo3114 added 2 commits August 27, 2017 17:03

- Pull from upstream

7383f1b

- Updated Test ConvertTo-HTML meta with invalid properties should thow

b82a9b6

dantraMSFT reviewed Aug 31, 2017

View reviewed changes

SteveL-MSFT requested changes Aug 31, 2017

View reviewed changes

ergo3114 added 2 commits August 31, 2017 18:09

- added {get;set;} for the Transitional vaiable

c0c4235

- Added warning message to meta tag from error

19f2ea2

SteveL-MSFT requested changes Sep 6, 2017

View reviewed changes

ergo3114 added 4 commits September 7, 2017 01:41

- Pull warning from resource file

a380a35

- Removed commented out code

4970e36

- Repush for Appveyor test

3a07e00

- Repushing after rebase

da80adc

SteveL-MSFT requested changes Sep 7, 2017

View reviewed changes

ergo3114 added 2 commits September 8, 2017 13:31

- Changed to pull warning message from resouce file

a0ac1a1

- Changed test for invalid meta keys to look for key pair in verbose …

605453f

…output

SteveL-MSFT approved these changes Sep 8, 2017

View reviewed changes

TravisEz13 approved these changes Sep 11, 2017

View reviewed changes

TravisEz13 merged commit bbc180a into PowerShell:master Sep 11, 2017

TravisEz13 added a commit to TravisEz13/PowerShell that referenced this pull request Sep 20, 2017

Revert pester portion of Added Meta, Charset, and Transitional parame…

cf2037d

…ters to ConvertTo-HTML (PowerShell#4184) (reverted from commit bbc180a)

TravisEz13 mentioned this pull request Sep 20, 2017

Revert pester portion of Added Meta, Charset, and Transitional parame… #4881

Merged

daxian-dbw pushed a commit that referenced this pull request Sep 20, 2017

Revert the changes to Pester from merged PR #4184 (#4881)

4c6ad4a

That PR incorrectly included Pester changes. Most likely because that individual did not update his submodules.

joeyaiello mentioned this pull request Oct 15, 2018

Docs needed for 'Added Meta, Charset, and Transitional parameters to ConvertTo-HTML' MicrosoftDocs/PowerShell-Docs#3095

Closed

joeyaiello removed Documentation Needed in this repo Documentation is needed in this repo labels Oct 15, 2018

Conversation

ergo3114 commented Jul 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented Jul 4, 2017

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!

TravisEz13 commented Jul 11, 2017

Uh oh!

TravisEz13 left a comment

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT Jul 16, 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!

ergo3114 Jul 16, 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!

SteveL-MSFT 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!

TravisEz13 commented Aug 21, 2017

Uh oh!

ergo3114 commented Aug 25, 2017

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!

ergo3114 commented Jul 4, 2017 •

edited

Loading

SteveL-MSFT Jul 16, 2017 •

edited

Loading

ergo3114 Jul 16, 2017 •

edited

Loading