Moving Import-PowerShellDatafile from script to compiled cmdlet by powercode · Pull Request #2750 · PowerShell/PowerShell · GitHub

Fixes #2734

Hi Staffan Gustafsson (@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;

Hemant Mahawar (@HemantMahawar) should we have copyright preamble on the new files?

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

removing

Please, avoid copy-paste

That's probably not needed

removed

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

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; }

Shouldn't ValueFromPipeline be on LiteralPath?

Corrected.

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.

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 Lee Holmes (@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.

Jason Shirk (@lzybkr) and sergei (@vors) Are your concerns resolved or is there additional work left here?

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.

Microsoft Contribution License Agreements (msftclas) added the cla-already-signed label Nov 21, 2016

Staffan Gustafsson (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

Hemant Mahawar (HemantMahawar) removed the Review-Needed label Dec 1, 2016

sergei (vors) reviewed Dec 4, 2016

View reviewed changes

sergei (vors) suggested changes Dec 4, 2016

View reviewed changes

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

Jason Shirk (lzybkr) suggested changes Jan 4, 2017

View reviewed changes

Jason Shirk (lzybkr) self-assigned this Jan 4, 2017

Staffan Gustafsson (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

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

Merge branch 'master' into import-datafile

1c2c3a2

Jason Shirk (lzybkr) reviewed Feb 27, 2017

View reviewed changes

Jason Shirk (lzybkr) approved these changes Feb 27, 2017

View reviewed changes

Jason Shirk (lzybkr) merged commit e3b59e0 into PowerShell:master Feb 27, 2017

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

Staffan Gustafsson (powercode) deleted the import-datafile branch April 19, 2017 17:28

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026

Moving Import-PowerShellDatafile from script to cmdlet (PowerShell#2750 )

2c95bc6

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

Moving Import-PowerShellDatafile from script to compiled cmdlet by powercode · Pull Request #2750 · PowerShell/PowerShell · GitHub

Uh oh!

Moving Import-PowerShellDatafile from script to compiled cmdlet - #2750

Moving Import-PowerShellDatafile from script to compiled cmdlet#2750Jason Shirk (lzybkr) merged 2 commits intoPowerShell:masterPowerShell/PowerShell:masterfrom powercode:import-datafilepowercode/PowerShell:import-datafileCopy head branch name to clipboard

Conversation

Staffan Gustafsson (powercode) commented Nov 21, 2016

Uh oh!

Microsoft Contribution License Agreements (msftclas) commented Nov 21, 2016

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!

sergei (vors) 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!

Staffan Gustafsson (powercode) Dec 6, 2016 • 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!

Staffan Gustafsson (powercode) Jan 4, 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!

Staffan Gustafsson (powercode) Jan 4, 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!

Mike Richmond (mirichmo) commented Feb 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Moving Import-PowerShellDatafile from script to compiled cmdlet#2750
Jason Shirk (lzybkr) merged 2 commits into
PowerShell:masterfrom
powercode:import-datafile

Staffan Gustafsson (powercode) Dec 6, 2016 •
edited
Loading

Staffan Gustafsson (powercode) Jan 4, 2017 •
edited
Loading

Staffan Gustafsson (powercode) Jan 4, 2017 •
edited
Loading