Skip to content

Fetch resource string correctly #5114

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
adityapatwardhan merged 1 commit into from
Oct 18, 2017
Merged

Fetch resource string correctly #5114

adityapatwardhan merged 1 commit into from
Oct 18, 2017

Conversation

@daxian-dbw
Copy link
Member

@daxian-dbw daxian-dbw commented Oct 14, 2017 •
edited
Loading

I found Add-Member throws error with an empty message. This PR fixes it.

Before fix

PS:2> Add-Member -InputObject $object "ABC" "Value1"
[C:\arena\pscore]
PS:3> Add-Member -InputObject $object "ABC" "Value1"
Add-Member :
At line:1 char:1
+ Add-Member -InputObject $object "ABC" "Value1"
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : InvalidOperation: (1 2:PSObject) [Add-Member], InvalidOperationException
    + FullyQualifiedErrorId : MemberAlreadyExists,Microsoft.PowerShell.Commands.AddMemberCommand

After fix

PS:8> Add-Member -InputObject $object "ABC" "Value1"
Add-Member : Cannot add a member with the name "ABC" because a member with that name already exists. To overwrite the
member anyway, add the Force parameter to your command.
At line:1 char:1
+ Add-Member -InputObject $object "ABC" "Value1"
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : InvalidOperation: (1 2:PSObject) [Add-Member], InvalidOperationException
    + FullyQualifiedErrorId : MemberAlreadyExists,Microsoft.PowerShell.Commands.AddMemberCommand

Also updated a resource string use in ConvertTo-Json to directly use the C# property.

iSazonov
iSazonov reviewed Oct 15, 2017
View reviewed changes
src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/ConvertToJsonCommand.cs
"WebCmdletStrings", "JsonStringInBadFormat");
ErrorRecord errorRecord = new ErrorRecord(
new InvalidOperationException(details.Message),
new InvalidOperationException(WebCmdletStrings.JsonStringInBadFormat),
Copy link
Collaborator

@iSazonov iSazonov Oct 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the change for the PR? I don't see a description and a test.

Copy link
Member Author

@daxian-dbw daxian-dbw Oct 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the resource file name and the resource string id is known/fixed, there is no point to use ResourceManager to retrieve the error message (which is what ErrorDetails does). Instead, we can just use the generated C# property.
The existence of the resource string is guaranteed by the build, so I didn't add a test for it.

Copy link
Collaborator

@iSazonov iSazonov Oct 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant that the PR description mention only Add-Member cmdlet but not ConvertTo-Json.

Copy link
Member Author

@daxian-dbw daxian-dbw Oct 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the description to include the ConvertTo-Json change.

Copy link
Contributor

@PaulHigin PaulHigin Oct 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error string for this resource (WebCmdletStrings.JsonStringInBadFormat) doesn't seem right:
"The converted JSON string is in bad format."
I think this would be a better error message:
"The JSON string could not be converted because the format is incorrect."

Copy link
Member Author

@daxian-dbw daxian-dbw Oct 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is from ConvertTo-Json cmdlet, which takes arbitrary objects and converts them to JSON string. The error message is used when it detects the JSON string we converted to is in bad format.

PaulHigin
PaulHigin approved these changes Oct 16, 2017
View reviewed changes
Copy link
Contributor

@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also update the WebCmdletStrings.JsonStringInBadFormat error string.

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/ConvertToJsonCommand.cs
"WebCmdletStrings", "JsonStringInBadFormat");
ErrorRecord errorRecord = new ErrorRecord(
new InvalidOperationException(details.Message),
new InvalidOperationException(WebCmdletStrings.JsonStringInBadFormat),
Copy link
Contributor

@PaulHigin PaulHigin Oct 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error string for this resource (WebCmdletStrings.JsonStringInBadFormat) doesn't seem right:
"The converted JSON string is in bad format."
I think this would be a better error message:
"The JSON string could not be converted because the format is incorrect."

@adityapatwardhan adityapatwardhan merged commit 2ceec72 into PowerShell:master Oct 18, 2017
@daxian-dbw daxian-dbw deleted the errorstring branch October 18, 2017 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes

@iSazonov iSazonov iSazonov approved these changes

@dantraMSFT dantraMSFT Awaiting requested review from dantraMSFT

@JamesWTruher JamesWTruher Awaiting requested review from JamesWTruher

+1 more reviewer

@PaulHigin PaulHigin PaulHigin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@adityapatwardhan adityapatwardhan

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@daxian-dbw @adityapatwardhan @PaulHigin @iSazonov