8000 Prettier formatting for ConvertTo-Json output. #2736 by kittholland · Pull Request #2787 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 5 commits into from
Feb 24, 2017

Conversation

kittholland
Copy link
Contributor
@kittholland kittholland commented Nov 26, 2016

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

@msftclas
Copy link
< 8000 /span>

Hi @kittholland, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@PowerShellTeam PowerShellTeam added the Review - Needed The PR is being reviewed label Nov 28, 2016
Copy link
Member
@SteveL-MSFT SteveL-MSFT left a 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.

@SteveL-MSFT
Copy link
Member

We should have a test for pretty print.

Copy link
Member
@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

LGTM

@kittholland
Copy link
Contributor Author

I will work on the pretty print test soon.

Copy link
Member
@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

LGTM

@SteveL-MSFT
Copy link
Member

@daxian-dbw the CI breaks are related to #2806, how to proceed?

@@ -0,0 +1,14 @@
Describe "ConvertTo-Json" -tags "CI" {
Copy link
Collaborator

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

Copy link
Contributor Author

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.

@daxian-dbw
Copy link
Member

@SteveL-MSFT I just restarted the Travis CI build.

@kittholland
Copy link
Contributor Author

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.

@lzybkr lzybkr added Review - Waiting on Author and removed Review - Needed The PR is being reviewed labels Dec 2, 2016
@PowerShellTeam PowerShellTeam added Review - Needed The PR is being reviewed and removed Review - Waiting on Author labels Dec 3, 2016
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.
@SteveL-MSFT
Copy link
Member

Closing and reopening to restart CI

@SteveL-MSFT SteveL-MSFT closed this Dec 9, 2016
@SteveL-MSFT SteveL-MSFT reopened this Dec 9, 2016
@msftclas
Copy link
msftclas commented Dec 9, 2016

Hi @kittholland, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@kittholland
Copy link
Contributor Author

Can we retry the travis CI? I have successful test results on ubuntu 14.04.

@SteveL-MSFT
Copy link
Member

Closing and reopening to restart CI

@SteveL-MSFT SteveL-MSFT closed this Dec 9, 2016
@SteveL-MSFT SteveL-MSFT reopened this Dec 9, 2016
@msftclas
Copy link
msftclas commented Dec 9, 2016

Hi @kittholland, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@kittholland
Copy link
Contributor Author
kittholland commented Dec 13, 2016

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.

@mirichmo
Copy link
Member
mirichmo commented Feb 9, 2017

@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?

@mirichmo mirichmo self-assigned this Feb 17, 2017
Copy link
Member
@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

LGTM

@mirichmo mirichmo merged commit d69193e into PowerShell:master Feb 24, 2017
@iSazonov iSazonov removed the Review - Needed The PR is being reviewed label Mar 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants
0