Add ShouldProcess to New-FileCatalog and Test-FileCatalog #3074
Conversation
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.
|
|
||
| Collection<string> paths = new Collection<string>(); | ||
|
|
||
| bool _ShouldProcess = false; |
There was a problem hiding this comment.
You don't need this boolean. You can use (paths.Count > 0) instead of (_ShouldProcess) to see if processing should continue.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@PaulHigin Good catch!
@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'?
There was a problem hiding this comment.
I removed _ShouldProcess.
| try | ||
| { | ||
| $null = New-FileCatalog -Path $sourcePath -CatalogFilePath $catalogPath -WhatIf | ||
| $result = Test-Path -Path ($catalogPath + "\catalog.cat") |
There was a problem hiding this comment.
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.
TravisEz13
added
Review - Waiting on Author
and removed
Review - Needed
PowerShellTeam
added
Review - Needed
Remove _ShouldProcess Add var in test
iSazonov
added
the
Documentation Needed in this repo
joeyaiello
removed
Documentation Needed in this repo
7 participants
Close #3068
Add support
-WhatIfand-ConfirmtoNew-FileCatalogand add atest.
Test-FileCataloghas a common code base withNew-FileCatalogso itautomatically get the same. I believe that adding a separate test in
this case doesn't make sense.