Added support for converting enums as strings in JSON #4318
Conversation
| @@ -464,6 +478,12 @@ private object ProcessValue(object obj, int depth) | |||
| { | |||
| rv = obj.ToString(); | |||
| } | |||
| #if !CORECLR | |||
There was a problem hiding this comment.
Why are you limiting this condition? I would expect EnumsAsString to apply to CORECLR.
If there is a specific reason, please add the comment.
There was a problem hiding this comment.
CORECLR uses a different mechanism to convert enums to strings (Newtonsoft.JSON has native support for this). If you look elsewhere in the PR you will see the enum converter conditionally applied for CORECLR. If you want this in comments in the code, I'll add them when I get back from vacation.
There was a problem hiding this comment.
We're removing FullCLR code, so don't add any here
There was a problem hiding this comment.
Ok, I removed that block. Let me know if anything else needs to be changed.
| $array = [pscustomobject]@{ objectName = "object1Name"; objectValue = "object1Value" }, | ||
| [pscustomobject]@{ objectName = "object2Name"; objectValue = "object2Value" } | ||
|
|
||
| $array = [pscustomobject]@{ objectName = "object1Name"; objectValue = "object1Value" }, |
There was a problem hiding this comment.
Unintended whitespace change?
There was a problem hiding this comment.
Yes. But I believe this is because the source newlines were inconsistent. I didn't apply any changes here nor in the next one. IMHO better for newlines to be consistent than to have to manually reset them when tooling does the right thing.
| @@ -1472,7 +1489,7 @@ Describe "Json Bug fixes" -Tags "Feature" { | |||
|
|
|||
| It "ConvertFrom-Json deserializes an array of strings (in multiple lines) as a single string." { | |||
|
|
|||
| $result = "[1,","2,","3]" | ConvertFrom-Json | |||
| $result = "[1,","2,","3]" | ConvertFrom-Json | |||
There was a problem hiding this comment.
Unintended whitespace change?
There was a problem hiding this comment.
Same reply as above. I did not make these changes, and believe them to be due to inconsistent newlines. My recommendation is to accept the change to make newlines consistent so that future changes in this file don't run into the same issue.
| @@ -464,6 +478,12 @@ private object ProcessValue(object obj, int depth) | |||
| { | |||
| rv = obj.ToString(); | |||
| } | |||
| #if !CORECLR | |||
There was a problem hiding this comment.
We're removing FullCLR code, so don't add any here
| @@ -60,6 +61,15 @@ public class ConvertToJsonCommand : PSCmdlet | |||
| [Parameter] | |||
| public SwitchParameter Compress { get; set; } | |||
|
|
|||
| /// <summary> | |||
| /// gets or sets the EnumsAsStrings property. | |||
There was a problem hiding this comment.
Please correct typo gets -> Gets.
|
|
||
| $response4 = ConvertTo-Json -InputObject $sampleObject -Compress -EnumsAsStrings | ||
| $response4 | Should Be '{"SampleSimpleEnum":"Ignore","SampleBitwiseEnum":"Alias, Function, Cmdlet"}' | ||
|
|
There was a problem hiding this comment.
Please remove extra line.
| $response4 | Should Be '{"SampleSimpleEnum":"Ignore","SampleBitwiseEnum":"Alias, Function, Cmdlet"}' | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
Please remove extra line.
|
@SteveL-MSFT: I'd like to get some clarification from the PowerShell Team on how you guys want PRs handled. This PR is done. It's tested and working, and all required changes have been completed. It just needs to be merged. Yet earlier today, @iSazonov added some additional requested changes that in my opinion add no additional value whatsoever to this PR. Speaking as someone who is extremely busy, yet makes time to submit PRs when I can, I find this very disrespectful of my time. The casing used in the comment before the added parameter matches the casing used in the comments before the other parameters in the file. Had I decided to change that and use "Gets", and at the same time changed the other comments to use "Gets" (for consistency), I would very likely have received a comment to undo the unnecessary changes that do not pert 8000 ain to this PR. And for the "extra blank line comments" requests -- why? Who says they are extra? Why is the blank line at the top of the test script block not also considered extra? I can see blank lines considered unnecessary when there are many in a row or when they are used a little too liberally in code, but when we're talking about a single blank line here and there that makes a file more readable, do we really need to nitpick on those details? If so, where are the rules written so that people like me who want to submit PRs can follow them from the onset and not waste time later with these insignificant details? IMHO these requested changes are absurd and have nothing to do with the PR at hand, so I am summarily ignoring them unless someone from the PowerShell team can provide me with a good reason to take time away from what I am otherwise busy doing in order to make these changes. Otherwise, if we can get back to focusing feedback on changes that are actually required for a PR to be approved, it would be appreciated. |
|
@KirkMunro All comments are welcome on PRs and issues and we have a process for handling scenarios like this although I am not sure if we ever documented it. I filed issue #4595 to cover the documentation hole. Ideally, if the changes requested are small, the author will just push a quick update to the PR and be done with it. If not, and the comments are non-blocking as they are in this case, the process is to file an issue covering the requested change and merge the PR. I filed issue #4596 to document the requested changes. |
|
@mirichmo I think your process is missing something. Should you really take all of the requested changes verbatim and throw them into an issue for someone else to address? Even the blank line changes? Really? That makes no sense. If I were to waste my time going through PRs and adding comments where I felt blank lines should be removed, does that mean you or someone else from the PowerShell team should create an issue suggesting someone in the community may want to remove those lines at some point? And for the single "Gets" comment, wouldn't it be more appropriate to log an issue suggesting that all of the comments where the string starts with lowercase be changed to start with uppercase, and not just one that happened to be entered as part of this PR in a manner consistent with the rest of the file? I don't see any value in #4596 at all -- it feels more like you're passing the time wasting on to someone else, and I feel there is an incorrect judgement being made with respect to what is an appropriate comment to add to a PR and what is not. Saying all comments are welcome is one thing, sure, but for people who are maintainers on a repository, I think the community needs to be able to expect better. |
|
@KirkMunro I am trying to establish a common process and behave in a predictable manner while also respecting the person submitting the PR and the person leaving the comments. Maybe @iSazonov feels strongly about those change requests and would like to do them himself? Maybe I should have just marked them as "Won't Fix" and forgotten the whole thing? I am learning here and your feedback is helpful. A better way to handle this scenario rather than just filing an issue would probably have been to leave a comment stating, "I do not consider these to be blocking comments that must be addressed as part of this PR. Please file an issue covering the comments if you would like to see them addressed." Also, I do not see issues as static things. Creating a separate issue allows for discussion and suggestions as well. Your suggestion regarding expanding #4596 to cover all strings like that does make it more interesting. |
|
Maybe rather than pull all comments that did not result in changes within a PR into an issue (which may just create undesirable noise), the right approach when closing a PR is for the author of the PR or the PS Team member merging/closing the PR to identify any optional changes that were not made and reply to those with a link that references an article (some anchored portion of a markdown file) that indicates that the proposed change was not required for the PR, but if they believe it is an important change to make they can file a separate issue requesting that change or they can open a PR with those changes. Then if the person proposing said changes really feels strongly that they should be made, they can take the appropriate steps to try to influence those changes. I wouldn't mind being able to drop in a simple hyperlink in response to any PR change requests that I am not in agreement with. I also think that document should discuss how to propose the right change and how to avoid proposing changes that are inappropriate to avoid all of this in the first place. |
|
@KirkMunro thanks for your feedback, I didn't respond earlier as I was out of the country on vacation. We as a team are still learning on how to properly engage with the community since becoming OSS. I agree that we want to make it as easy as possible for the community to submit PRs and get them merged, but also easy for the community and team to provide feedback against those PRs. Let me discuss it with the team and we'll share out thoughts on how to improve the process and experience. |
|
@KirkMunro I hope I addressed your concerns with this PR #4710 |
|
I saw that earlier today. Thanks @SteveL-MSFT, that did address my concerns. |
There are many scenarios where the default serialization behaviour of
ConvertTo-Jsonfor enumerated types is undesirable. JSON does not contain type information for the properties contained within it, and as a result deserialization of JSON enumerations into a meaningful representation (such as their string representation) is impossible.This PR adds a new
-EnumsAsStringsparameter to theConvertTo-Jsoncmdlet to provide an alternative serialization option that converts all enumerations to their string representation as part of the serialization process, so that the data remains meaningful when the JSON string is deserialized later.