FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix LiteralPath in Import-Csv to bind to Get-ChildItem output by iSazonov · Pull Request #8277 · PowerShell/PowerShell · GitHub

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

Merged
Ilya (iSazonov) merged 7 commits into
PowerShell:masterfrom
iSazonov:literalpath-import-csv
Jan 9, 2019
Merged

Fix LiteralPath in Import-Csv to bind to Get-ChildItem output#8277
Ilya (iSazonov) merged 7 commits into
PowerShell:masterfrom
iSazonov:literalpath-import-csv

Conversation

Ilya (iSazonov) commented Nov 15, 2018
edited
Loading

Copy link
Copy Markdown
Collaborator

PR Summary

Fix #4473.

Now works:

dir | Import-Csv

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

PR Checklist

Ilya (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
Ilya (iSazonov) self-assigned this Nov 15, 2018

Copy link
Copy Markdown
Collaborator Author

cc Michael Klement (@mklement0)

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Michael Klement (mklement0) Nov 16, 2018
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Michael Klement (@mklement0) Thanks! I removed the ValueFromPipelineByPropertyName from Path parameter.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
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).

Steve Lee (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

Copy link
Copy Markdown
Collaborator

Michael Klement (@mklement0) code seems to address issues, Ilya (@iSazonov) have you had a chance to review it?

Ilya (iSazonov) removed the Breaking-Change breaking change that may affect users label Nov 29, 2018

Copy link
Copy Markdown
Collaborator Author

Done.

Copy link
Copy Markdown
Collaborator Author

Other cmdlets share parameter set names. Need more investigations.

Ilya (iSazonov) force-pushed the literalpath-import-csv branch 4 times, most recently from dc30923 to 887b131 Compare December 6, 2018 11:11

Copy link
Copy Markdown
Collaborator Author

Michael Klement (@mklement0) James Truher (@JamesWTruher) Please update your code review.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is there already an existing test case for the example Michael Klement (@mklement0) brought up?

'path' | import-csv

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I did not understand your question. Previously the 'path' | import-csv example did not work.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm referring to Michael Klement (@mklement0)'s comment here #8277 (comment)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ilya (@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
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What is new code path we cover by the test?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I will add new test in the PR..

Ilya (iSazonov) force-pushed the literalpath-import-csv branch 2 times, most recently from 867c892 to 309a9ee Compare January 9, 2019 12:24

Copy link
Copy Markdown
Collaborator Author

Michael Klement (@mklement0) Steve Lee (@SteveL-MSFT) New tests was added. Please update your review.

Ilya (iSazonov) merged commit 6647b29 into PowerShell:master Jan 9, 2019
Ilya (iSazonov) deleted the literalpath-import-csv branch January 9, 2019 13:49
Ilya (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
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL