Join GitHub today
GitHub is home to over 36 million developers working together to host and review code, manage projects, and build software together.
Sign upGet/Add-Content throws improved error when targeting a container #7823
Conversation
kvprasoon
requested review from
anmenaga and
BrucePay
as
code owners
Sep 19, 2018
This comment has been minimized.
This comment has been minimized.
|
@iSazonov I think more rules are added to CodeFactor, issues are in untouched lines. |
iSazonov
reviewed
Sep 20, 2018
| // Throw error if path is a directory. | ||
| if (Directory.Exists(path)) | ||
| { | ||
| string errMsg = StringUtil.Format(SessionStateStrings.GetContainerContentException, path); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
kvprasoon
Sep 20, 2018
Author
Contributor
Added different error message for Write to container exception.
| // Users can't both read as bytes, and specify a delimiter | ||
| if (delimiterSpecified) | ||
| { | ||
| { |
This comment has been minimized.
This comment has been minimized.
| @@ -6366,9 +6366,18 @@ public IContentReader GetContentReader(string path) | |||
|
|
|||
| try | |||
| { | |||
| // Throw error if path is a directory. | |||
This comment has been minimized.
This comment has been minimized.
| @@ -6514,6 +6523,15 @@ public IContentWriter GetContentWriter(string path) | |||
|
|
|||
| try | |||
| { | |||
| // Throw error if path is a directory. | |||
This comment has been minimized.
This comment has been minimized.
iSazonov
reviewed
Sep 21, 2018
| </data> | ||
| </data> | ||
| <data name="WriteContainerContentException" xml:space="preserve"> | ||
| <value>Unable to access '{0}' contents as it is a directory.</value> |
This comment has been minimized.
This comment has been minimized.
iSazonov
Sep 21, 2018
Collaborator
I'd prefer {0} on end of the string because we support long paths.
Unable to write content because it is a directory: {0}For GetContainerContentException too.
/cc @SteveL-MSFT
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
SteveL-MSFT
Sep 21, 2018
Member
I think the updated error messages are fine, even for GetContainerContentException.
This comment has been minimized.
This comment has been minimized.
|
The package validation on NJsonSchema is a known issue not related to this PR |
This comment has been minimized.
This comment has been minimized.
|
Reopen the PR to restart all CIs. |
iSazonov
closed this
Sep 23, 2018
iSazonov
reopened this
Sep 23, 2018
This comment has been minimized.
This comment has been minimized.
|
@kvprasoon can you rebase off master to pick up test fixes? |
iSazonov
self-assigned this
Sep 25, 2018
kvprasoon
added some commits
Sep 19, 2018
This comment has been minimized.
This comment has been minimized.
|
@kvprasoon I rebased for you. Further please create new branch in your fork for every new change (don't put commits in forked master). |
This comment has been minimized.
This comment has been minimized.
|
@iSazonov Sure, will do it, learning OSS collaboration from each PR... thanks |
kvprasoon commentedSep 19, 2018
PR Summary
Fix #7770
Fix #2686
Enhancing the error message as per #7770 and #2686
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests