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

Fix LiteralPath in Import-Csv to bind to Get-ChildItem output #8277

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@iSazonov
Collaborator

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

This comment has been minimized.

Collaborator

iSazonov commented Nov 15, 2018

@mklement0

This comment has been minimized.

Contributor

mklement0 commented Nov 15, 2018

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.

@@ -512,7 +512,7 @@ public sealed
/// <summary>
/// Mandatory file name to read from.
/// </summary>
[Parameter(Position = 0, ValueFromPipeline = true)]
[Parameter(Position = 0, ValueFromPipelineByPropertyName = true)]

This comment has been minimized.

@mklement0

mklement0 Nov 15, 2018

Contributor

For consistency with the other cmdlets, it is LiteralPath that should be bound, not Path.

This comment has been minimized.

@iSazonov

iSazonov Nov 16, 2018

Collaborator

I used Get-Content as sample. Can you point better sample?

This comment has been minimized.

@mklement0

mklement0 Nov 16, 2018

Contributor

Search for instances of [Alias("PSPath")] in the source code. If Get-Content behaves differently, that's a bug that should be fixed too.

This comment has been minimized.

@iSazonov

iSazonov Nov 16, 2018

Collaborator

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.

This comment has been minimized.

@mklement0

mklement0 Nov 16, 2018

Contributor

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.

This comment has been minimized.

@iSazonov

iSazonov Nov 16, 2018

Collaborator

@mklement0 Thanks! I removed the ValueFromPipelineByPropertyName from Path parameter.

@@ -512,7 +512,7 @@ public sealed
/// <summary>
/// Mandatory file name to read from.
/// </summary>
[Parameter(Position = 0, ValueFromPipeline = true)]
[Parameter(Position = 0)]

This comment has been minimized.

@mklement0

mklement0 Nov 16, 2018

Contributor

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

This comment has been minimized.

Member

SteveL-MSFT commented Nov 29, 2018

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

@JamesWTruher

This comment has been minimized.

Member

JamesWTruher commented Nov 29, 2018

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

@iSazonov

This comment has been minimized.

Collaborator

iSazonov commented Nov 29, 2018

Done.

@iSazonov

This comment has been minimized.

Collaborator

iSazonov commented Nov 29, 2018

Other cmdlets share parameter set names. Need more investigations.

@iSazonov iSazonov force-pushed the iSazonov:literalpath-import-csv branch 3 times, most recently from cf0ddec to dc30923 Dec 6, 2018

@iSazonov iSazonov force-pushed the iSazonov:literalpath-import-csv branch from dc30923 to 887b131 Dec 6, 2018

@iSazonov

This comment has been minimized.

Collaborator

iSazonov commented Dec 6, 2018

@mklement0 @JamesWTruher Please update your code review.

//if delimiter is not given, it should take , as value
if (Delimiter == '\0')
{
Delimiter = ImportExportCSVHelper.CSVDelimiter;
}

break;
case "UseCulture":
case "CulturePath":

This comment has been minimized.

@mklement0

mklement0 Dec 6, 2018

Contributor

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.

This comment has been minimized.

@iSazonov

iSazonov Dec 6, 2018

Collaborator

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment