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 — Add ShouldProcess to New-FileCatalog and Test-FileCatalog by iSazonov · Pull Request #3074 · PowerShell/PowerShell

Conversation

@iSazonov

@iSazonov Ilya (iSazonov) commented Jan 30, 2017

Copy link
Copy Markdown
Collaborator

Close #3068

Add support -WhatIf and -Confirm to New-FileCatalog and add a
test.
Test-FileCatalog has a common code base with New-FileCatalog so it
automatically get the same. I believe that adding a separate test in
this case doesn't make sense.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@iSazonov
Add ShouldProcess to New-FileCatalog and Test-FileCatalog
2b2c4f3 
Close PowerShell#3068

Add support `-WhatIf` and `-Confirm` to `New-FileCatalog` and add a
test.
`Test-FileCatalog` has a common code base with `New-FileCatalog` so it
automatically get the same. I believe that adding a separate test in
this case doesn't make sense.
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-already-signed label Jan 30, 2017
@PowerShellTeam PowerShellTeam added the Review - Needed The PR is being reviewed label Jan 30, 2017
@TravisEz13
Travis Plunk (TravisEz13) requested a review from FayzaSultan January 30, 2017 22:57
@TravisEz13 Travis Plunk (TravisEz13) self-assigned this Jan 30, 2017
PaulHigin
Paul Higinbotham (PaulHigin) reviewed Feb 3, 2017
View reviewed changes
Comment thread src/Microsoft.PowerShell.Security/security/CatalogCommands.cs Outdated

Collection<string> paths = new Collection<string>();

bool _ShouldProcess = false;

@PaulHigin Paul Higinbotham (PaulHigin) Feb 2, 2017

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.

You don't need this boolean. You can use (paths.Count > 0) instead of (_ShouldProcess) to see if processing should continue.

Sorry, something went wrong.

Uh oh!

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

@lzybkr Jason Shirk (lzybkr) Feb 4, 2017

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.

Paul's suggestion is good, but I'm fine with a local variable as well - but the local variable should not use the convention for fields - it should be shouldProcess.

Sorry, something went wrong.

Uh oh!

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

@iSazonov Ilya (iSazonov) Feb 4, 2017

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.

Paul Higinbotham (@PaulHigin) Good catch!
Jason Shirk (@lzybkr) If we take into consideration that cmdlets can work with large directories, then maybe we will remove the local variable and add a comment 'paths.Count > 0 is true only if ShouldProcess allow'?

Sorry, something went wrong.

Uh oh!

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

@iSazonov Ilya (iSazonov) Feb 16, 2017

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

Sorry, something went wrong.

Uh oh!

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

@TravisEz13 Travis Plunk (TravisEz13) Feb 16, 2017

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.

Paul Higinbotham (@PaulHigin) I think your comment has been addressed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
lzybkr
Jason Shirk (lzybkr) reviewed Feb 4, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Security/FileCatalog.Tests.ps1 Outdated
try
{
$null = New-FileCatalog -Path $sourcePath -CatalogFilePath $catalogPath -WhatIf
$result = Test-Path -Path ($catalogPath + "\catalog.cat")

@lzybkr Jason Shirk (lzybkr) Feb 4, 2017

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 would introduce a variable $catalogFile or something like that and use the variable when trying to delete the file.

I would also probably not call Remove-File if Test-Path says the file isn't there.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
TravisEz13 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@iSazonov Ilya (iSazonov) Feb 16, 2017

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.
All reactions
@TravisEz13 Travis Plunk (TravisEz13) added Review - Waiting on Author and removed Review - Needed The PR is being reviewed labels Feb 16, 2017
@PowerShellTeam PowerShellTeam added Review - Needed The PR is being reviewed and removed Review - Waiting on Author labels Feb 16, 2017
@iSazonov
Fiz after code review
b7a02e6 
Remove  _ShouldProcess
Add var in test
TravisEz13
Travis Plunk (TravisEz13) approved these changes Feb 16, 2017
View reviewed changes
@TravisEz13
Travis Plunk (TravisEz13) merged commit e10cbff into PowerShell:master Feb 18, 2017
@iSazonov
Ilya (iSazonov) deleted the newfilecatalogwhatif branch February 20, 2017 07:15
@iSazonov Ilya (iSazonov) removed the Review - Needed The PR is being reviewed label Mar 27, 2017
@iSazonov Ilya (iSazonov) added the Documentation Needed in this repo Documentation is needed in this repo label Jun 17, 2017
@markekraus Mark Kraus (markekraus) mentioned this pull request Dec 1, 2017
Test-FileCatalog does not work on PSCore. #5594
Closed
@joeyaiello Joey Aiello (joeyaiello) mentioned this pull request Oct 15, 2018
Docs needed for 'Add ShouldProcess to New-FileCatalog and Test-FileCatalog' MicrosoftDocs/PowerShell-Docs#3107
Closed
@joeyaiello Joey Aiello (joeyaiello) removed Documentation Needed in this repo Documentation is needed in this repo labels Oct 15, 2018
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@iSazonov @TravisEz13
Add ShouldProcess to New-FileCatalog and Test-FileCatalog (PowerShell…
2ae4df6 
…#3074)

* Add ShouldProcess to New-FileCatalog and Test-FileCatalog

Close PowerShell#3068

Add support `-WhatIf` and `-Confirm` to `New-FileCatalog` and add a
test.
`Test-FileCatalog` has a common code base with `New-FileCatalog` so it
automatically get the same. I believe that adding a separate test in
this case doesn't make sense.

* Fiz after code review

Remove  _ShouldProcess
Add var in test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TravisEz13 Travis Plunk (TravisEz13) TravisEz13 approved these changes
@FayzaSultan FayzaSultan Awaiting requested review from FayzaSultan
+2 more reviewers
@lzybkr Jason Shirk (lzybkr) lzybkr left review comments
@PaulHigin Paul Higinbotham (PaulHigin) PaulHigin left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

@TravisEz13 Travis Plunk (TravisEz13)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

7 participants

@iSazonov @lzybkr @TravisEz13 @PaulHigin @joeyaiello @PowerShellTeam @msftclas
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.