Skip to content
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 FileCatalog cmdlets to work if -Path is not specified #5596

Merged
merged 1 commit into from
Dec 1, 2017

Conversation

SteveL-MSFT
Copy link
Member

Remove unnecessary check for Paths.count > 0 as there is code later to use the current working directory since -Path is not a mandatory parameter.
Updated ShouldProcess to output the internal action on adding paths rather than the user action (which is the cmdlet name).

Updated tests to not specify -Path

Fix #5594

… use the current

working directory since -Path is not a mandatory parameter

Updated tests to not specify -Path
Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor

@markekraus markekraus left a comment

Choose a reason for hiding this comment

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

LGTM

@TravisEz13 TravisEz13 merged commit b69ff71 into PowerShell:master Dec 1, 2017
@TravisEz13
Copy link
Member

Reviewing for RC2 triage: Should process should not hardcode message string

@TravisEz13 TravisEz13 modified the milestones: 6.0.0-GA, 6.0.0-RC.2 Dec 2, 2017
TravisEz13 pushed a commit to TravisEz13/PowerShell that referenced this pull request Dec 2, 2017
… use the current (PowerShell#5596)

Remove unnecessary check for Paths.count > 0 as there is code later to use the current working directory since -Path is not a mandatory parameter.
Updated ShouldProcess to output the internal action on adding paths rather than the user action (which is the cmdlet name).

Updated tests to not specify -Path

Fix PowerShell#5594
@SteveL-MSFT SteveL-MSFT deleted the test-catalog branch December 2, 2017 00:30
TravisEz13 pushed a commit that referenced this pull request Dec 2, 2017
… use the current (#5596)

Remove unnecessary check for Paths.count > 0 as there is code later to use the current working directory since -Path is not a mandatory parameter.
Updated ShouldProcess to output the internal action on adding paths rather than the user action (which is the cmdlet name).

Updated tests to not specify -Path

Fix #5594

if (ShouldProcess(catalogFilePath))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for being late. Above we made "Including path " + tempPath.ProviderPath but here we show only path catalogFilePath - different style message.

Copy link
Member Author

Choose a reason for hiding this comment

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

ShouldProcess() here automatically shows the cmdlet name as the action, so it says something like: What If: Executing Test-FileCatalog against "catalogFilePath"

Copy link
Collaborator

Choose a reason for hiding this comment

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

My question is about the difference - if an user set both Path and CatalogFilePath parameters he can get some requests for files from Path and then different style request for catalogFilePath - he can not understand that last request is for just catalog file.

Copy link
Member Author

@SteveL-MSFT SteveL-MSFT Dec 4, 2017

Choose a reason for hiding this comment

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

If both is specified, you see this:

PS C:\Users\steve\test> New-FileCatalog -Path C:\Users\steve\test,c:\,c:\Users\steve -CatalogFilePath c:\users\steve\test\test.cat -WhatIf
What if: Including path C:\Users\steve\test
What if: Including path C:\
What if: Including path C:\Users\steve
What if: Performing the operation "New-FileCatalog" on target "c:\users\steve\test\test.cat".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants