Skip to content

Conversation

@iSazonov
Copy link
Collaborator

@iSazonov iSazonov commented Nov 15, 2018

PR Summary

Fix #4473.

Now works:

dir | Import-Csv

From source issue discussion the behavior should be "by design" like Get-Content.

PR Checklist

@iSazonov iSazonov added Review - Committee The PR/Issue needs a review from the PowerShell Committee Breaking-Change breaking change that may affect users CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log labels Nov 15, 2018
@iSazonov iSazonov self-assigned this Nov 15, 2018
@iSazonov iSazonov requested a review from SteveL-MSFT November 15, 2018 12:02
@iSazonov
Copy link
Collaborator Author

cc @mklement0

@mklement0
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Collaborator Author

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?

Copy link
Contributor

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.

Copy link
Collaborator Author

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.

Copy link
Contributor

@mklement0 mklement0 Nov 16, 2018

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.

Copy link
Collaborator Author

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.

Copy link
Contributor

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

@SteveL-MSFT
Copy link
Member

@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).

@SteveL-MSFT SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Nov 29, 2018
@JamesWTruher
Copy link
Collaborator

@mklement0 code seems to address issues, @iSazonov have you had a chance to review it?

@iSazonov iSazonov removed the Breaking-Change breaking change that may affect users label Nov 29, 2018
@iSazonov
Copy link
Collaborator Author

Done.

@iSazonov
Copy link
Collaborator Author

Other cmdlets share parameter set names. Need more investigations.

@iSazonov iSazonov force-pushed the literalpath-import-csv branch 4 times, most recently from dc30923 to 887b131 Compare December 6, 2018 11:11
@iSazonov
Copy link
Collaborator Author

iSazonov commented Dec 6, 2018

@mklement0 @JamesWTruher Please update your code review.

Copy link
Contributor

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Member

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-csv

Copy link
Collaborator Author

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.

Copy link
Member

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)

Copy link
Collaborator Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iSazonov:

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.

Copy link
Collaborator Author

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?

Copy link
Contributor

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-Item input.

  • Tests currently missing:

  • 'path' | import-csv (as before)

  • [pscustomobject] @{ Path = 'path' } | Import-Csv (newly added)

  • [pscustomobject] @{ LiteralPath = 'path' } | Import-Csv (as before)

Copy link
Member

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.

Copy link
Collaborator Author

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..

@iSazonov iSazonov force-pushed the literalpath-import-csv branch 2 times, most recently from 867c892 to 309a9ee Compare January 9, 2019 12:24
@iSazonov iSazonov force-pushed the literalpath-import-csv branch from 309a9ee to dc4a905 Compare January 9, 2019 12:47
@iSazonov
Copy link
Collaborator Author

iSazonov commented Jan 9, 2019

@mklement0 @SteveL-MSFT New tests was added. Please update your review.

@iSazonov iSazonov merged commit 6647b29 into PowerShell:master Jan 9, 2019
@iSazonov iSazonov deleted the literalpath-import-csv branch January 9, 2019 13:49
@iSazonov iSazonov added Breaking-Change breaking change that may affect users CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log labels Jan 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Breaking-Change breaking change that may affect users CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log Committee-Reviewed PS-Committee has reviewed this and made a decision

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Import-Csv fails to bind Get-ChildItem output to its -LiteralPath parameter via the pipeline

4 participants