Don't write an error if file already unblocked #5362
Conversation
iSazonov
requested review from
JamesWTruher,
PaulHigin and
dantraMSFT
as code owners
iSazonov
force-pushed
the
fix-error-in-unblock-file
branch
from
c6a244a to
668028b
Compare
lzybkr
left a comment
lzybkr
left a comment
There was a problem hiding this comment.
I see there are existing tests for "file does not exist" and they still pass, so this change looks good.
| WriteError(new ErrorRecord(accessException, "RemoveItemUnauthorizedAccessError", ErrorCategory.PermissionDenied, path)); | ||
| // NativeErrorCode=2 - File not found. | ||
| // If the block stream not found the 'path' was not blocked and we successfully return. | ||
| if (accessException.NativeErrorCode != 2) |
There was a problem hiding this comment.
@jpsnover suggests adding a verbose message here if the block stream is not found
There was a problem hiding this comment.
Done. I tried to avoid adding a resource string
There was a problem hiding this comment.
I think it would be better to add a resource string saying:
File '{0}' did not contain a 'Zone.Identifier' file stream and was already not blocked.
There was a problem hiding this comment.
I added a resouce string with {0} at last position - we should take into account long paths.
| WriteError(new ErrorRecord(accessException, "RemoveItemUnauthorizedAccessError", ErrorCategory.PermissionDenied, path)); | ||
| // NativeErrorCode=2 - File not found. | ||
| // If the block stream not found the 'path' was not blocked and we successfully return. | ||
| if (accessException.NativeErrorCode != 2) |
There was a problem hiding this comment.
I think it would be better to add a resource string saying:
File '{0}' did not contain a 'Zone.Identifier' file stream and was already not blocked.
| <value>'{0}' is not supported in this system.</value> | ||
| </data> | ||
| <data name="NoZoneIdentifierFileStream" xml:space="preserve"> | ||
| <value>"No 'Zone.Identifier' file stream. The file was already not blocked: {0}"</value> |
There was a problem hiding this comment.
Should this be present tense e.g. "The file is already not blocked: ..."? Maybe ask one of your documentation folks about the wording for this error message? Sorry but error messages in PowerShell really need help and I'd rather we not contribute to the problem with poor error messages. Do you have a "doc/grammar" person to review error messages?
There was a problem hiding this comment.
The file is already unblocked:
There was a problem hiding this comment.
@rkeithhill unfortunately at this time, our previous doc person left Msft. Doc changes aren't breaking, so we can continue to improve them. I agree reading it again that it should probably be present tense: The file is already not blocked.
@markekraus perhaps I'm a bit pedantic on this one, but unblocked implies the file was once blocked which may not have been the case here.
There was a problem hiding this comment.
@SteveL-MSFT You're right, and what Keith wrote is more accurate. but already not blocked sounds odd.. maybe The file is not blocked?
There was a problem hiding this comment.
@markekraus agree, that sounds better: The file is not blocked.
SteveL-MSFT
left a comment
SteveL-MSFT
left a comment
There was a problem hiding this comment.
@iSazonov can you make the requested string change?
|
Yes, I agree. |
|
@iSazonov thinking about it, I think it may be better to exclude that implementation detail as it may confuse some users. I think just: Is good. |
|
Done. |
6 participants
Fix #5353