-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Fix LiteralPath in Import-Csv to bind to Get-ChildItem output #8277
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
cc @mklement0 |
|
Thanks for taking this on, @iSazonov. Given that the current behavior is useless, I would consider this a straightforward bug fix, however, not a breaking change. |
There was a problem hiding this comment.
Choose a reason for 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.
There was a problem hiding this comment.
Choose a reason for 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?
There was a problem hiding this comment.
Choose a reason for 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.
There was a problem hiding this comment.
Choose a reason for 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.
There was a problem hiding this comment.
Choose a reason for 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mklement0 Thanks! I removed the ValueFromPipelineByPropertyName from Path parameter.
There was a problem hiding this comment.
Choose a reason for 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-Csvcurrently works: -
also making the parameter
ValueFromPipelineByPropertyNamemay make sense, because many other cmdlets support it (includingGet-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|
@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). |
|
@mklement0 code seems to address issues, @iSazonov have you had a chance to review it? |
|
Done. |
|
Other cmdlets share parameter set names. Need more investigations. |
dc30923 to
887b131
Compare
|
@mklement0 @JamesWTruher Please update your code review. |
There was a problem hiding this comment.
Choose a reason for 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for 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 @mklement0 brought up?
'path' | import-csvThere was a problem hiding this comment.
Choose a reason for 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm referring to @mklement0's comment here #8277 (comment)
There was a problem hiding this comment.
Choose a reason for 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.
There was a problem hiding this comment.
Choose a reason for 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.
There was a problem hiding this comment.
Choose a reason for 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?
There was a problem hiding this comment.
Choose a reason for 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-Iteminput. -
Tests currently missing:
-
'path' | import-csv(as before) -
[pscustomobject] @{ Path = 'path' } | Import-Csv(newly added) -
[pscustomobject] @{ LiteralPath = 'path' } | Import-Csv(as before)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 @mklement0 outlined above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add new test in the PR..
867c892 to
309a9ee
Compare
309a9ee to
dc4a905
Compare
|
@mklement0 @SteveL-MSFT New tests was added. Please update your review. |
PR Summary
Fix #4473.
Now works:
From source issue discussion the behavior should be "by design" like
Get-Content.PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests