Moving Import-PowerShellDatafile from script to compiled cmdlet #2750

powercode · 2016-11-21T19:06:31Z

msftclas · 2016-11-21T19:06:35Z

Hi @powercode, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

vors · 2016-12-04T20:41:15Z

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

@@ -0,0 +1,122 @@
+/********************************************************************++
+Copyright (c) Microsoft Corporation.  All rights reserved.
+--********************************************************************/


@HemantMahawar should we have copyright preamble on the new files?

Given it is open-source code, I don't think it is needed. @joeyaiello Can you weigh in?

vors

Please, avoid copy-paste

vors · 2016-12-04T20:43:40Z

src/Microsoft.PowerShell.Commands.Utility/map.json

@@ -47,6 +47,7 @@
    "monad/src/commands/utility/GetUnique.cs":  "commands/utility/GetUnique.cs",
    "monad/src/commands/utility/group-object.cs":  "commands/utility/group-object.cs",
    "monad/src/commands/utility/ImplicitRemotingCommands.cs":  "commands/utility/ImplicitRemotingCommands.cs",
+    "monad/src/commands/utility/ImportPowerShellDataFile.cs":  "commands/utility/ImportPowerShellDataFile.cs",


That's probably not needed

vors · 2016-12-04T20:47:14Z

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

+            }
+        }
+
+        private List<string> ResolveFilePaths(string[] filePaths, bool isLiteralPath)


This method is copied from

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

Line 1723 in 9a195f4

private List<string> ResolveFilePaths(string[] filePaths, bool isLiteralPath)

Can we avoid such copy-paste?

I really agree, but I actually think it would be a good Idea to provide these kind of helpers in a public API. This needs to be done by everyone who wants to handle Path and LiteralPath.

What is the right way to go here?

See #2729

lzybkr · 2016-12-07T01:03:09Z

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

+        public string[] LiteralPath
+        {
+            get { return Path; }
+            set { _isLiteralPath = true; Path = value; }


Do we use this pattern of sharing the backing store for parameters elsewhere? If not, it seems like potentially a bad idea to start doing this.

One maybe non-issue is the parameter binder copies all parameters in the cmdlet instance before parameter binding and restores them. It seems unlikely, but possible, that a non-null value in Path could cause problems.

A quick search gives that the pattern is in

AclCommands.cs CertificateCommands.cs CSVCommands.cs ExportAliasCommand.cs MatchString.cs Out-File.cs SaveHelpCommand.cs SignatureCommands.cs StartTranscriptCmdlet.cs UpdateHelpCommand.cs WriteFormatDataCommand.cs XmlCommands.cs

But the cost of changing is very small.

I changed it to

get { return _isLiteralPath ? Path : null; }

lzybkr · 2016-12-07T01:04:57Z

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

+        [Parameter(Mandatory = true,
+            Position = 0,
+            ParameterSetName = "ByPath",
+            ValueFromPipeline = true,


Shouldn't ValueFromPipeline be on LiteralPath?

lzybkr · 2016-12-07T01:07:12Z

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

+
+        void WriteInvalidDataFileError(string resolvedPath, string extraError)
+        {
+            var errorId = $"CouldNotParseAsPowerShellDataFile{extraError}";


I realize this is just a translation of the PowerShell, but it seems overly fancy. I think it would be clearer if the caller just passed errorId, it would make searching for the error string easier too.

corrected.

Rebased version pushed.

lzybkr · 2017-01-04T00:43:00Z

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

+                    }
+                    else
+                    {
+                        var data = ast.Find(a => a is HashtableAst, false);


I realize this is just a translation of the PowerShell implementation, but a psd1 can contain much more than a simple hashtable. For example, the psd1 file contain 2 hashtables as an array, and this implementation would ignore the second one.

As implemented, the cmdlet is mostly useful for typical module manifests, but psd1 files are used for other things, e.g. localization, and sometimes use commands like ConvertFrom-StringData.

We should, as a minimum, not ignore any part of the ast.
We should also consider supporting more than a simple hashtable. Ast.SafeGetValue can certainly handle more (e.g. an array of hashtables).
We might also consider doing something more like what Import-LocalizedData does, though we'd need feedback from @LeeHolmes to understand the important differences and why this cmdlet was introduced.

Should we start with just the move as descripted in the issue, and open a new issue for the functional changes?

+1 don't block the PR and open new Issue.

mirichmo · 2017-02-09T02:24:09Z

@lzybkr and @vors Are your concerns resolved or is there additional work left here?

lzybkr · 2017-02-27T00:47:29Z

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

+        [Parameter(Mandatory = true, Position = 0, ParameterSetName = "ByPath")]        
+        [ValidateNotNullOrEmpty]
+        [SuppressMessage("Microsoft.Performance", "CA1819:PropertiesShouldNotReturnArrays")]
+        public string[] Path { get; set; }


We don't need a change as this code is just following a somewhat broken pattern.

If LiteralPath was set, _isLiteralPath will be set to true. Then, if Path is set, _isLiteralPath is not cleared. This isn't really a scenario I guess, but it is handled correctly in some places in our code base, but in many others it is not.

msftclas added the cla-already-signed label Nov 21, 2016

powercode force-pushed the import-datafile branch from 520dced to 60da0d9 Compare November 21, 2016 21:59

PowerShellTeam added the Review - Needed label Nov 21, 2016

HemantMahawar removed the Review-Needed label Dec 1, 2016

vors reviewed Dec 4, 2016

View reviewed changes

vors suggested changes Dec 4, 2016

View reviewed changes

powercode force-pushed the import-datafile branch 4 times, most recently from 2470581 to ba64f8f Compare December 6, 2016 21:38

lzybkr suggested changes Jan 4, 2017

View reviewed changes

lzybkr self-assigned this Jan 4, 2017

powercode force-pushed the import-datafile branch 3 times, most recently from 6186748 to bc2b766 Compare January 4, 2017 22:37

Moving Import-PowerShellDatafile from script to cmdlet

141cce0

powercode force-pushed the import-datafile branch from bc2b766 to 141cce0 Compare January 4, 2017 22:40

Merge branch 'master' into import-datafile

1c2c3a2

lzybkr reviewed Feb 27, 2017

View reviewed changes

lzybkr approved these changes Feb 27, 2017

View reviewed changes

lzybkr merged commit e3b59e0 into PowerShell:master Feb 27, 2017

iSazonov removed the Review - Needed The PR is being reviewed label Mar 27, 2017

powercode deleted the import-datafile branch April 19, 2017 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moving Import-PowerShellDatafile from script to compiled cmdlet #2750

Moving Import-PowerShellDatafile from script to compiled cmdlet #2750

Moving Import-PowerShellDatafile from script to compiled cmdlet #2750

Moving Import-PowerShellDatafile from script to compiled cmdlet #2750

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment