Group-Object - Use Case-Sensitive Hashtable for -CaseSensitive -AsHashtable #11030
Conversation
- Group-Object -AsHashtable -CaseSensitive returns a case-sensitive hash instead of erroring out when given keys differing only by case. - Added tests
There was a problem hiding this comment.
I think we can simplify the code and this helps us to add Culture parameter later #9348:
var comparer = CaseSensitive.IsPresent ? StringComparer.CurrentCulture : StringComparer.CurrentCultureIgnoreCase;
hashtable = Hashtable(comparer);There was a problem hiding this comment.
Can do! I'm not sure if these methods use currentculture or ordinal though. I'll check :)
EDIT: It's CurrentCulture. 😄
Fixed in next commit. 🙂
There was a problem hiding this comment.
Please move style fixes in another PR. B 7440 elow too.
There was a problem hiding this comment.
VS code's autoformat strikes again. It is in a separate commit at least, but if you'd prefer to do a whole separate PR I can manage. 🙂
test/powershell/Modules/Microsoft.PowerShell.Utility/Group-Object.Tests.ps1
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Utility/Group-Object.Tests.ps1
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.Commands.Utility/commands/utility/Group-Object.cs
Outdated
Show resolved
Hide resolved
- Consolidate hashtable creation - Better consistency of handling switches in conditionals
b94fffc to
c307f2e
Compare
|
@iSazonov I think that should address all your concerns. I'll look at doing a style pass over the tests and possible the cmdlet after this PR is completed. 😊 |
|
@vexx32 I don't know should we document this in docs repo? |
|
🤔 I suppose it should be documented since we're changing established behaviour. I'll open an issue. EDIT: Done! |
test/powershell/Modules/Microsoft.PowerShell.Utility/Group-Object.Tests.ps1
Outdated
Show resolved
Hide resolved
…ect.Tests.ps1 Co-Authored-By: Steve Lee <slee@microsoft.com>
|
Hello @iSazonov! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
@SteveL-MSFT 🤔 do you know if the bot ignores non-required checks? Pretty sure it's waiting on the CodeFactor issue (which is just "complex method" on EndProcessing() here). I'm happy to refactor it out into helper methods if you feel that's necessary. 🙂 |
|
🎉 Handy links: |
PR Summary
Prior to this change,
Group-Object -AsHashtable -CaseSensitivewould give a key duplication error when given entries that only differ by casing. This was due to always using a case-insensitive hashtable, despite the request for-CaseSensitivebehaviour.This change allows
Group-Object -CaseSensitive -AsHashtableto create and return a case-sensitive hashtable to enable this use case.PR Context
Resolves #10441
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.