GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||
GeezerVison is showing github.com — Fix LiteralPath in Import-Csv to bind to Get-ChildItem output by iSazonov · Pull Request #8277 · PowerShell/PowerShell Conversation
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks for taking this on, Ilya (@iSazonov). Given that the current behavior is useless, I would consider this a straightforward bug fix, however, not a breaking change. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
For consistency with the other cmdlets, it is LiteralPath that should be bound, not Path.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I used Get-Content as sample. Can you point better sample?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Search for instances of [Alias("PSPath")] in the source code. If Get-Content behaves differently, that's a bug that should be fixed too.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Do you suggest remove ValueFromPipelineByPropertyName from Path parameter?
I found the same in WSMAnInstance (Set-WSManInstance).
I think it is not related to the PR and we need to review this in depth in an issue if it is important.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
All cmdlets that have both -Path and -LiteralPath parameters should have a PSPath alias defined for -LiteralPath and have ValueFromPipelineByPropertyName set to true.
That way, pipeline input from Get-Item / Get-ChildItem binds to -LiteralPath, as it should. By contrast, -Path is for wildcard-based paths.
While fixing other cmdlets that do not comply with this pattern is outside the scope of this PR, making Import-Csv comply is in scope.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Michael Klement (@mklement0) Thanks! I removed the ValueFromPipelineByPropertyName from Path parameter.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
My apologies: I missed a few things:
by removing ValueFromPipeline, we actually take away existing functionality, because something like '/path/to/file.csv' | Import-Csv currently works:
also making the parameter ValueFromPipelineByPropertyName may make sense, because many other cmdlets support it (including Get-Content, as you told me, but I was focused on the wrong aspect).
So I think the right fix is to introduce new parameter sets that separate the -Path and -LiteralPath parameters, analogous to how the other cmdlets do it. The challenge here is that they must be combined with the existing Delimiter and UseCulture parameter sets.
Here's code that should work:
/// <summary> /// Implements Import-Csv command. /// </summary> [Cmdlet(VerbsData.Import, "Csv", DefaultParameterSetName = "DelimiterPath", HelpUri = "https://go.microsoft.com/fwlink/?LinkID=113341")] public sealed class ImportCsvCommand : PSCmdlet { #region Command Line Parameters /// <summary> /// Property that sets delimiter. /// </summary> [Parameter(ParameterSetName = "DelimiterPath", Position = 1)] [Parameter(ParameterSetName = "DelimiterLiteralPath", Position = 1)] [ValidateNotNull] public char Delimiter { get; set; } /// <summary> /// Mandatory file name to read from. /// </summary> [Parameter(ParameterSetName = "DelimiterPath", Position = 0, Mandatory = true, ValueFromPipeline = true, ValueFromPipelineByPropertyName = true)] [Parameter(ParameterSetName = "CulturePath", Position = 0, Mandatory = true, ValueFromPipeline = true, ValueFromPipelineByPropertyName = true)] [ValidateNotNullOrEmpty] public String[] Path { get { return _paths; } set { _paths = value; _specifiedPath = true; } } private string[] _paths; private bool _specifiedPath = false; /// <summary> /// The literal path of the mandatory file name to read from. /// </summary> [Parameter(ParameterSetName = "DelimiterLiteralPath", Mandatory = true, ValueFromPipelineByPropertyName = true)] [Parameter(ParameterSetName = "CultureLiteralPath", Mandatory = true, ValueFromPipelineByPropertyName = true)] [ValidateNotNullOrEmpty] [Alias("PSPath", "LP")] [SuppressMessage("Microsoft.Performance", "CA1819:PropertiesShouldNotReturnArrays")] public string[] LiteralPath { get { return _paths; } set { _paths = value; _isLiteralPath = true; } } private bool _isLiteralPath = false; /// <summary> /// Property that sets UseCulture parameter. /// </summary> [Parameter(ParameterSetName = "CulturePath", Mandatory = true)] [Parameter(ParameterSetName = "CultureLiteralPath", Mandatory = true)] [ValidateNotNull] public SwitchParameter UseCulture { get { return _useculture; } set { _useculture = value; } } private bool _useculture; ///<summary> /// Header property to customize the names. ///</summary> [Parameter(Mandatory = false)] [ValidateNotNullOrEmpty] [SuppressMessage("Microsoft.Performance", "CA1819:PropertiesShouldNotReturnArrays")] public string[] Header { get; set; } /// <summary> /// Encoding optional flag. /// </summary> [Parameter()] [ArgumentToEncodingTransformationAttribute()] [ArgumentEncodingCompletionsAttribute] [ValidateNotNullOrEmpty] public Encoding Encoding { get; set; } = ClrFacade.GetDefaultEncoding(); /// <summary> /// Avoid writing out duplicate warning messages when there are one or more unspecified names. /// </summary> private bool _alreadyWarnedUnspecifiedNames = false; #endregion Command Line Parameters
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@PowerShell/powershell-committee reviewed this and the current change is breaking and doesn't need to be. The current behavior needs to be retained (and tested). |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Michael Klement (@mklement0) code seems to address issues, Ilya (@iSazonov) have you had a chance to review it? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Done. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Other cmdlets share parameter set names. Need more investigations. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
dc30923 to
887b131
Compare
|
Michael Klement (@mklement0) James Truher (@JamesWTruher) Please update your code review. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
If I understand this correctly, "UseCulture" should be added back in, because this method is used from the Export-Csv and ConvertTo-Csv cmdlets too, which still have the original parameter set names, Delimiter and UseCulture.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Is there already an existing test case for the example Michael Klement (@mklement0) brought up?
'path' | import-csv
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I did not understand your question. Previously the 'path' | import-csv example did not work.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I'm referring to Michael Klement (@mklement0)'s comment here #8277 (comment)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The fix come from the comment. I see no reason to add an explicit test for the fix, so I simply changed the existing test.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The 'path' | import-csv example did work, it was just piping from Get-ChildItem / Get-Item that was broken.
Also, given that we've now added support for [pscustomobject] @{ Path = 'path' } | Import-Csv, I think we should test that too.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
What is new code path we cover by the test?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I guess it comes down to whether we want to test all aspects of the expected pipeline-binding behavior (I don't know what the policy is and how this is handled for other cmdlets).
If we do:
You've already added a test for binding Get-ChildItem / Get-Item input.
Tests currently missing:
'path' | import-csv (as before)
[pscustomobject] @{ Path = 'path' } | Import-Csv (newly added)
[pscustomobject] @{ LiteralPath = 'path' } | Import-Csv (as before)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Ilya (@iSazonov) the concern is that we didn't have adequate test coverage here and the previous commit caused a regression. If this was a big ask, I would have it as a separate PR/issue, however, it seems like a small item to add some test cases for those Michael Klement (@mklement0) outlined above.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I will add new test in the PR..
Sorry, something went wrong.
There was an error while loading. Please reload this page.
867c892 to
309a9ee
Compare
309a9ee to
dc4a905
Compare
|
Michael Klement (@mklement0) Steve Lee (@SteveL-MSFT) New tests was added. Please update your review. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
Import-Csv fails to bind Get-ChildItem output to its -LiteralPath parameter via the pipeline
There was an error while loading. Please reload this page.