Skip to content

Replace StringComparision.CurrentCulture with StringComparision.Ordinal #8068

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.

Sign up for GitHub

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

Jump to bottom
Merged
iSazonov merged 25 commits into from
Oct 21, 2018
Merged

Replace StringComparision.CurrentCulture with StringComparision.Ordinal #8068

iSazonov merged 25 commits into from
Oct 21, 2018

Conversation

@iSazonov
Copy link
Collaborator

@iSazonov iSazonov commented Oct 18, 2018 •
edited by TravisEz13
Loading

PR Summary

Related #8064

Replace StringComparision.CurrentCulture with StringComparision.Ordinal.

Best Practices for Using Strings in .NET

Not all code base is fixed. There are not obvious cases.

PR Checklist

This change is Reviewable

@iSazonov iSazonov added the Issue-Code Cleanup the issue is for cleaning up the code with no impact on functionality label Oct 18, 2018
@iSazonov iSazonov self-assigned this Oct 18, 2018
@iSazonov iSazonov requested a review from SteveL-MSFT October 18, 2018 14:29
@TravisEz13
Copy link
Member

TravisEz13 commented Oct 18, 2018

src/Microsoft.WSMan.Management/WSManConnections.cs, line 143 at r1 (raw file):

            {
                computername = value;
                if ((string.IsNullOrEmpty(computername)) || (computername.Equals(".", StringComparison.OrdinalIgnoreCase)))

I've seen this in many places, perhaps we should have a central place for this code?

TravisEz13
TravisEz13 reviewed Oct 18, 2018
View reviewed changes
Copy link
Member

@TravisEz13 TravisEz13 left a comment

Choose a reason for hiding this comment

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

Reviewed 24 of 24 files at r1.
Reviewable status: :shipit: complete! all files reviewed

@TravisEz13
Copy link
Member

TravisEz13 commented Oct 18, 2018

We probably should fix this one as well:

PowerShell/src/System.Management.Automation/engine/parser/SafeValues.cs

Line 641 in 2f1863a

Hashtable hashtable = new Hashtable(StringComparer.CurrentCultureIgnoreCase);

@TravisEz13
Copy link
Member

TravisEz13 commented Oct 18, 2018

Thinking about it further with @JamesWTruher , Hashtable could be culture sensitive in some cases. This should be a different PR and probably be behind an experimental flag.

@iSazonov
Copy link
Collaborator Author

iSazonov commented Oct 19, 2018

Hashtable could be culture sensitive in some cases.

@TravisEz13 In the PR I fixed only obvious cases. There are still about 30 unclear cases left. It would be great if MSFT team reviewed them.

I've seen this in many places, perhaps we should have a central place for this code?

If we should please open a tracking issue and I'll fix this later.

@iSazonov iSazonov merged commit 0a4f33a into PowerShell:master Oct 21, 2018
@iSazonov iSazonov deleted the cleanup-culture branch October 21, 2018 11:11
@iSazonov iSazonov mentioned this pull request Dec 31, 2018
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TravisEz13 TravisEz13 TravisEz13 approved these changes

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw daxian-dbw is a code owner

@powercode powercode Awaiting requested review from powercode

@SteveL-MSFT SteveL-MSFT Awaiting requested review from SteveL-MSFT

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan adityapatwardhan is a code owner

@anmenaga anmenaga Awaiting requested review from anmenaga

@BrucePay BrucePay Awaiting requested review from BrucePay

@Francisco-Gamino Francisco-Gamino Awaiting requested review from Francisco-Gamino

@JamesWTruher JamesWTruher Awaiting requested review from JamesWTruher

Assignees

@iSazonov iSazonov

Labels

Issue-Code Cleanup the issue is for cleaning up the code with no impact on functionality

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@iSazonov @TravisEz13