GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Fix LiteralPath in Import-Csv to bind to Get-ChildItem output by iSazonov · Pull Request #8277 · PowerShell/PowerShell

Conversation

@iSazonov

@iSazonov Ilya (iSazonov) commented Nov 15, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Change is not breaking
  • Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
  • This PR is ready to merge and is not Work in Progress.
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.
  • User-facing changes
    • Not Applicable
    • OR
    • User-facing Documentation needed
      • Issue filed - Issue link:
  • Testing - New and feature
    • Not Applicable or can only be tested interactively
    • OR
    • Make sure you've added a new test if existing tests do not effectively test the code changed
      • Add [feature] if the change is significant or affects feature tests

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@iSazonov 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
@iSazonov Ilya (iSazonov) self-assigned this Nov 15, 2018
@iSazonov
Ilya (iSazonov) requested review from James Truher (JamesWTruher) and Paul Higinbotham (PaulHigin) as code owners November 15, 2018 12:01
@iSazonov
Ilya (iSazonov) requested a review from Steve Lee (SteveL-MSFT) November 15, 2018 12:02
@iSazonov

Ilya (iSazonov) commented Nov 15, 2018

Copy link
Copy Markdown
Collaborator Author

cc Michael Klement (@mklement0)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mklement0

Michael Klement (mklement0) commented Nov 15, 2018

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mklement0
Michael Klement (mklement0) suggested changes Nov 15, 2018
View reviewed changes
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/CSVCommands.cs Outdated

@mklement0 Michael Klement (mklement0) Nov 15, 2018

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@iSazonov Ilya (iSazonov) Nov 16, 2018

Copy link
Copy Markdown
Collaborator Author

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@mklement0 Michael Klement (mklement0) Nov 16, 2018

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@iSazonov Ilya (iSazonov) Nov 16, 2018

Copy link
Copy Markdown
Collaborator Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@mklement0 Michael Klement (mklement0) Nov 16, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@iSazonov Ilya (iSazonov) Nov 16, 2018

Copy link
Copy Markdown
Collaborator Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mklement0 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
mklement0
Michael Klement (mklement0) suggested changes Nov 16, 2018
View reviewed changes
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/CSVCommands.cs Outdated

@mklement0 Michael Klement (mklement0) Nov 16, 2018

Copy link
Copy Markdown
Contributor

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

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Nov 29, 2018

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@SteveL-MSFT 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
@JamesWTruher

James Truher (JamesWTruher) commented 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?

mklement0 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Ilya (iSazonov) commented Nov 29, 2018

Copy link
Copy Markdown
Collaborator Author

Done.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iSazonov

Ilya (iSazonov) commented Nov 29, 2018

Copy link
Copy Markdown
Collaborator Author

Other cmdlets share parameter set names. Need more investigations.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Ilya (iSazonov) commented Dec 6, 2018

Copy link
Copy Markdown
Collaborator Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mklement0
Michael Klement (mklement0) suggested changes Dec 6, 2018
View reviewed changes
Comment thread src/Microsoft.PowerShell.Commands.Utility/commands/utility/CSVCommands.cs Outdated

@mklement0 Michael Klement (mklement0) Dec 6, 2018

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@iSazonov Ilya (iSazonov) Dec 6, 2018

Copy link
Copy Markdown
Collaborator Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mklement0 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
mklement0
Michael Klement (mklement0) approved these changes Dec 6, 2018
View reviewed changes
SteveL-MSFT
Steve Lee (SteveL-MSFT) reviewed Dec 20, 2018
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/Import-Csv.Tests.ps1 Outdated

@SteveL-MSFT Steve Lee (SteveL-MSFT) Dec 20, 2018

Copy link
Copy Markdown
Member

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 Michael Klement (@mklement0) brought up?

'path' | import-csv

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@iSazonov Ilya (iSazonov) Dec 20, 2018

Copy link
Copy Markdown
Collaborator Author

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 did not understand your question. Previously the 'path' | import-csv example did not work.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Dec 20, 2018

Copy link
Copy Markdown
Member

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 Michael Klement (@mklement0)'s comment here #8277 (comment)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@iSazonov Ilya (iSazonov) Dec 20, 2018

Copy link
Copy Markdown
Collaborator Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@mklement0 Michael Klement (mklement0) Dec 20, 2018

Copy link
Copy Markdown
Contributor

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@iSazonov Ilya (iSazonov) Dec 20, 2018

Copy link
Copy Markdown
Collaborator Author

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@mklement0 Michael Klement (mklement0) Dec 20, 2018

Copy link
Copy Markdown
Contributor

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

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Dec 21, 2018

Copy link
Copy Markdown
Member

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.

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.

Uh oh!

There was an error while loading. Please reload this page.

mklement0 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@iSazonov Ilya (iSazonov) Dec 21, 2018

Copy link
Copy Markdown
Collaborator Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

SteveL-MSFT reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
SteveL-MSFT
Steve Lee (SteveL-MSFT) approved these changes Dec 21, 2018
View reviewed changes
Ilya (iSazonov) added 3 commits January 9, 2019 16:11
@iSazonov
Fix LiteralPath in Import-Csv to bind to Get-ChildItem output
5f35649
@iSazonov
Remove ValueFromPipelineByPropertyName from Path parameter
2eefba0
@iSazonov
mklement0's change
482446f
Ilya (iSazonov) added 3 commits January 9, 2019 16:14
@iSazonov
Fix parameter set names
a3c14fc
@iSazonov
Fix parametersets in Export-Csv
dc73865
@iSazonov
Revert SetDelimiter()
db3376f
@iSazonov
Ilya (iSazonov) force-pushed the literalpath-import-csv branch 2 times, most recently from 867c892 to 309a9ee Compare January 9, 2019 12:24
@iSazonov
Add new tests
dc4a905
@iSazonov
Ilya (iSazonov) force-pushed the literalpath-import-csv branch from 309a9ee to dc4a905 Compare January 9, 2019 12:47
@iSazonov

Ilya (iSazonov) commented Jan 9, 2019

Copy link
Copy Markdown
Collaborator Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mklement0
Michael Klement (mklement0) approved these changes Jan 9, 2019
View reviewed changes
@iSazonov
Ilya (iSazonov) merged commit 6647b29 into PowerShell:master Jan 9, 2019
@iSazonov
Ilya (iSazonov) deleted the literalpath-import-csv branch January 9, 2019 13:49
@iSazonov 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
This was referenced Aug 22, 2019
Doc that you can now pipe files to Import-Csv MicrosoftDocs/PowerShell-Docs#4676
Closed
Rename default ParameterSetName back to Delimiter for ConvertTo-Csv & ConvertFrom-Csv #10425
Merged
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@iSazonov
Fix LiteralPath in Import-Csv to bind to Get-ChildItem output (PowerS…
ffd0571
…hell#8277)

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

Reviewers

@SteveL-MSFT Steve Lee (SteveL-MSFT) SteveL-MSFT approved these changes
@JamesWTruher James Truher (JamesWTruher) Awaiting requested review from JamesWTruher
@PaulHigin Paul Higinbotham (PaulHigin) Awaiting requested review from PaulHigin
+1 more reviewer
@mklement0 Michael Klement (mklement0) mklement0 approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@iSazonov Ilya (iSazonov)

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

Milestone

No milestone

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

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@iSazonov @mklement0 @SteveL-MSFT @JamesWTruher
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.