-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Change hashtable to use OrdinalIgnoreCase to be case-insensitive in all Cultures #8566
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
|
I'm pretty sure this is a breaking change. |
Could you please reference where it come from and why? Is there other cultures like |
|
@iSazonov read the details in the related issue |
|
Perhaps it makes sense to consider wider the culture-insensitive and case-insensitive in PowerShell Committee review. Related #8064 (Not all code base fixed as mentioned in #8068). |
|
@iSazonov In my opinion this pull request correctly fixes the issue described in #7761 and we should keep wider discussion of changing other culture-insensitive in other places out of this pull request. As this is a serious issue in a core functionality (Hashtable) and as it is present on the default culture of Ubuntu 18.04 (C.utf8) and thus exposed to a large number of users in my opinion this should be merged with high priority and be backported to 6.1.X and 6.0.X. |
|
My request is not to block the PR. My fear is that there may be other places prone to this problem. This analysis may be useful for fixing other issues. |
ffeldhaus
left a comment
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.
Issues also affects Mac OS, so test should run on Mac OS as well.
test/powershell/Language/Parser/LanguageAndParser.TestFollowup.Tests.ps1
Outdated
Show resolved
Hide resolved
….Tests.ps1 Co-Authored-By: SteveL-MSFT <slee@microsoft.com>
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.
Looks good to me, only a minor, optional suggestion to improve readability of the test :)
test/powershell/Language/Parser/LanguageAndParser.TestFollowup.Tests.ps1
Outdated
Show resolved
Hide resolved
ffeldhaus
left a comment
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.
Addresses the issue and test is good as well now!
….Tests.ps1 Co-Authored-By: SteveL-MSFT <slee@microsoft.com>
|
@PowerShell/powershell-committee reviewed this and agrees to take this change as the intention was to always have Hashtable keys case-insensitive |
|
@iSazonov Only one |
|
Wow, thank you for fixing this. I am new to PowerShell Core and using it on Linux, and it seemed that this was intentional. I sent a lot of time fixing the casing of my hash tables. I'm guessing this is also the issue I ran into with sort-object, which gives different results on Linux and Windows: PS> $PSVersionTable.Platform;('a', 'ab', 'aC' | Sort-Object ) -join ','
Win32NT
a,ab,aC
PS> $PSVersionTable.Platform;('a', 'ab', 'aC' | Sort-Object ) -join ','
Unix
a,aC,ab |
PR Summary
Ubuntu18.04 seems to default to
C.UTF-8for LANG (representing InvariantCulture) which results in a case-sensitive hashtable since CurrentCultureIgnoreCase doesn't work for that culture. Fix is to use OrdinalIgnoreCase instead.Fix #7761
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests