Do not reject Windows' reserved device names on non-Windows platforms. #3252
Conversation
There was a problem hiding this comment.
Is there a more specific check that it failed because of reserved name vs something else?
There was a problem hiding this comment.
The current code throws an IOException with a localized string as its message, and uses "CopyError" ("MoveError"/"RenameError") in the error record.
We could modify the error ID in the error record to say something like "CopyToReservedNameError", with "MoveTo" and "RenameTo" versions, if that wouldn't badly impact existing user code.
There was a problem hiding this comment.
It is in common function PathIsReservedDeviceName
We could make new specific IOException but is it worth?
There was a problem hiding this comment.
$tempFile seems not necessary in these tests. $testFile is created in BeforeEach and removed in AfterEach, so you can use testFile directly here.
There was a problem hiding this comment.
For Move-Item and Rename-Item the $tempFile was to be able to restore $testFile each time through the foreach across device names. I can certainly use New-Item to create a new $testFile each iteration if you prefer.
There was a problem hiding this comment.
Ah, right. It's in a loop. Ignore my comment. #Closed
There was a problem hiding this comment.
How about just Test-Path $deviceName | Should Be $true?
There was a problem hiding this comment.
I was just keeping consistency with existing tests. I'm happy to make this change. Should I do so for the pre-existing tests as well?
There was a problem hiding this comment.
I see. You can submit a new PR later to address that particular issue. #Closed
There was a problem hiding this comment.
I suppose that's enough:
Copy-Item -Path $testFile -Destination $deviceName -Force -ErrorAction SilentlyContinue
Test-Path $deviceName | Should Be $trueThe same for tests below.
There was a problem hiding this comment.
@jeffbi It would be great if you can address this comment, but it doesn't block merging this PR in case you want to address it later for all existing tests in this file. Please let me know.
There was a problem hiding this comment.
I've addressed @iSazonov's comments here. I'll put off changing the previously existing tests.
There was a problem hiding this comment.
Maybe more specifically:
"Copy-Item succeeds on Unix with Windows reserved device names"
There was a problem hiding this comment.
Yeah, I agree. I'll change that.
| It "Move-Item on Unix succeeds with Windows reserved device names" -Skip:($IsWindows) { | ||
| foreach ($deviceName in $reservedNames) | ||
| { | ||
| Copy-Item -Path $testFile -Destination $tempFile -Force |
There was a problem hiding this comment.
Maybe really use New-Item here? The code will be more clear.
5 participants
Fixes #3221