Skip to content

Prettier formatting for ConvertTo-Json output. #2736 - #2787

Merged
Mike Richmond (mirichmo) merged 5 commits into
PowerShell:masterfrom
kittholland:ConvertTo-JSON
Feb 24, 2017
Merged

Mike Richmond (mirichmo) merged 5 commits into
PowerShell:masterfrom
kittholland:ConvertTo-JSON

Conversation

@kittholland

@kittholland Kitt Holland (kittholland) commented Nov 26, 2016

Copy link
Copy Markdown
Contributor

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

Hi Kitt Holland (@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;

@SteveL-MSFT Steve Lee (SteveL-MSFT) left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Member

We should have a test for pretty print.

@SteveL-MSFT Steve Lee (SteveL-MSFT) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@kittholland

Copy link
Copy Markdown
Contributor Author

I will work on the pretty print test soon.

@SteveL-MSFT Steve Lee (SteveL-MSFT) left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Member

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

Steve Lee (@SteveL-MSFT) I just restarted the Travis CI build.

@kittholland

Copy link
Copy Markdown
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.

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
Copy Markdown
Member

Closing and reopening to restart CI

@msftclas

Hi Kitt Holland (@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
Copy Markdown
Contributor Author

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

@SteveL-MSFT

Copy link
Copy Markdown
Member

Closing and reopening to restart CI

@msftclas

Hi Kitt Holland (@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

Kitt Holland (kittholland) commented Dec 13, 2016

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Member

Steve Lee (@SteveL-MSFT) and James Truher (@JamesWTruher) This looks like a forgotten PR that is a candidate for merging. Can you please take a second look at it?

@SteveL-MSFT Steve Lee (SteveL-MSFT) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@mirichmo
Mike Richmond (mirichmo) merged commit d69193e into PowerShell:master Feb 24, 2017
@iSazonov Ilya (iSazonov) removed the Review - Needed The PR is being reviewed label Mar 27, 2017
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
…Shell#2787)

* Prettier formatting for ConvertTo-Json output. PowerShell#2736

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

* Updated CoreCLR implementation to use NewtonSoft Indented Formatting

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.

* Added tests that validate pretty Json output.

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 tests and removed extraneous file.

Moved pretty/compressed json tests from standalone file into the
existing ConvertTo-Json test file.

* Updated tests for cross-platform support
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