-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
Change ConvertFrom-Json -AsHashtable
to use ordered hashtable
#17405
Conversation
Going to rework this as additional |
ConvertFrom-Json -AsHashable
to use ordered dictionaryConvertFrom-Json -AsHashable
to use ordered dictionary
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
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.
This implementation seems unusual to me. Everything is computed as an ordered dictionary, but then at the end converted to a Hashtable type if requested. Why not just have two code paths, one for Hashtable switch and one for Ordered switch?
src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs
Outdated
Show resolved
Hide resolved
Given the ubiquity of ContainsKey and ContainsValue when dealing with Hashtables I think this is probably a very large breaking change if it does not sit behind a -switch on the cmdlet so I also vote for having it as a switch (not that I get a vote). |
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment. |
0147e54
to
0c3f08b
Compare
ConvertFrom-Json -AsHashable
to use ordered dictionaryConvertFrom-Json -AsHashable
to use ordered hashtable
ConvertFrom-Json -AsHashable
to use ordered hashtableConvertFrom-Json -AsHashtable
to use ordered hashtable
CI bootstrap issue is dotnet-install.sh no longer working, created dotnet/install-scripts#285 |
ConvertFrom-Json -AsHashtable
to use ordered hashtableConvertFrom-Json -AsHashtable
to use ordered hashtable
@PaulHigin can you take another look, decided instead of a separate switch for Hashtable vs OrderedDictionary, just wrap OrderedDictionary as OrderedHashtable to preserve compatibility |
src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/OrderedHashtable.cs
Outdated
Show resolved
Hide resolved
e84cd2b
to
135122f
Compare
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
There are multiple ways that the order of data might be valuable, so losing that is a negative and changing to keep it is a positive. There's just a question of possible breakages, and since Windows PowerShell didn't have the -AsHashTable switch changing its behaviour doesn't affect 15 years' worth of scripts. Anything which uses it in PowerShell 6 /7 is able to work with the items in any order so if the order is the original order it doesn't matter. A few things can't work with the randomized order or need to do extra work. One pitfall is side stepped by using this new ordered hashtable instead of an ordered dictionary - anything that checks inpu with |
src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/OrderedHashtable.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/OrderedHashtable.cs
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertFrom-Json.Tests.ps1
Show resolved
Hide resolved
@SteveL-MSFT Need rebase to pass Unix tests? |
526b6af
to
2125725
Compare
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
🎉 Handy links: |
PR Summary
JSON typically has ordering to make it easier to read. Currently, a Hashtable does not preserve ordering so for complex JSON, the members are not where you would expect in terms of order. This change wraps OrderedDictionary into a OrderedHashtable that derives from Hashtable. This preserves existing compatibility with scripts that expect a Hashtable to be returned, but preserves the order of elements.
PR Context
Fix #17404
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).