-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Prettier formatting for ConvertTo-Json output. #2736 #2787
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.
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
Conversation
Hi @kittholland, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! The agreement was validated by Microsoft and real humans are currently evaluating your PR. TTYL, MSBOT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On CoreCLR, I would prefer to use the NewtonSoft Json serializer with Format=Formatting.Indented. For FullCLR, I would just leave it as-is today. The compiler should leave out all the custom formatting code for a CoreCLR build, otherwise we can #ifdef
it.
We should have a test for pretty print. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I will work on the pretty print test soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@daxian-dbw the CI breaks are related to #2806, how to proceed? |
@@ -0,0 +1,14 @@ | |||
Describe "ConvertTo-Json" -tags "CI" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Describe [](start = 0, length = 8)
please add these to test\powershell\Modules\Microsoft.PowerShell.Utility\pester.commands.cmdlets.json.tests.ps1 rather than creating a new file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking in fix now, thanks for pointing out the correct location.
@SteveL-MSFT I just restarted the Travis CI build. |
Thanks for rerunning Travis-CI, I see my tests are failing in that environment. I will have to build out a non windows environment for more testing. There are also some here-string based tests that are labeled -pending$isCoreCLR and are tagged in a Feature describe block. I think I will need to rework any here-string based tests to accomodate the new formatting. |
cde9dcf
to
3606650
Compare
This change standardizes JSON output to example given, as well as codemaid and online lint tools. Sample object used for testing: @{ foo = @{ first = 'a' second = 'bbbbbbbb' } barbarbarbar = @{ first = 'a' second = 'bbbbbbbb' NestedArray = @( 'Test3' 'Test4' 'Test5' 3 4 ) NestedObject = @{ MoreObject = 'AnotherObject' TestBool = $true } } array = @( 'Thing1' 'Thing2' ) dan = 15 } | ConvertTo-Json
I did not change the FullCLR behavior, I was not sure if you meant to revert my changes or to leave it as is in the current pull request.
Not sure if there is a better thought on how to implement these. The first two fail against current master, but succeed once this PR is applied. Third test is successful prior and post this PR.
Moved pretty/compressed json tests from standalone file into the existing ConvertTo-Json test file.
3606650
to
9790487
Compare
Closing and reopening to restart CI |
Hi @kittholland, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! The agreement was validated by Microsoft and real humans are currently evaluating your PR. TTYL, MSBOT; |
Can we retry the travis CI? I have successful test results on ubuntu 14.04. |
Closing and reopening to restart CI |
Hi @kittholland, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! The agreement was validated by Microsoft and real humans are currently evaluating your PR. TTYL, MSBOT; |
Due to differences in newline characters between platforms, I think doing comparisons on here-strings will have to be very carefully handled if it is possible. Currently these tests are marked with Feature tag and $pending:($IsCoreCLR) so I think its ok. |
@SteveL-MSFT and @JamesWTruher This looks like a forgotten PR that is a candidate for merging. Can you please take a second look at it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This change standardizes JSON output to example given, as well as
codemaid and online lint tools.
Sample object used for testing: