Don't write an error if file already unblocked - #5362
Conversation
c6a244a to
668028b
Compare
Jason Shirk (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.
Jeffrey Snover (@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.
Keith Hill (@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.
Mark Kraus (@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.
Steve Lee (@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.
Mark Kraus (@markekraus) agree, that sounds better: The file is not blocked.
Steve Lee (SteveL-MSFT)
left a comment
There was a problem hiding this comment.
Ilya (@iSazonov) can you make the requested string change?
|
Yes, I agree. |
|
Ilya (@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. |
Fix #5353