Skip to content
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

Code Health | Enforcing Ordinal for StringComparison #2068

Merged
merged 4 commits into from Jun 27, 2023

Conversation

JRahnama
Copy link
Member

@Wraith2: based on the other conversation we had on PR #2065, what do you think on this change request?

@ErikEJ
Copy link
Contributor

ErikEJ commented Jun 20, 2023

I was just going to ask if there were other potential relevant changes - LGTM

@Wraith2
Copy link
Contributor

Wraith2 commented Jun 20, 2023

Looks like a good idea to me.

.editorconfig Outdated Show resolved Hide resolved
@David-Engel
Copy link
Contributor

To add background for anyone investigating this in the future: In a library like MDS, almost all string comparison operations being done inside the library should not use culture comparison rules (they should use Ordinal). For example, if we are parsing a connection string, we are looking for specific characters, not culture-specific representations of them. For example, if we are looking for a comma (also a thousands separator in some languages), we only want to find a comma, not a period (also a thousands separator in some languages). Default string comparison operations in .NET are culture specific. To be safe, we should always be explicit with which option we want to use inside MDS.

@DavoudEshtehari DavoudEshtehari added this to the 5.2.0-preview3 milestone Jun 20, 2023
@DavoudEshtehari DavoudEshtehari added the ➕ Code Health Changes related to source code improvements label Jun 20, 2023
@DavoudEshtehari DavoudEshtehari added this to In progress in SqlClient v5.2 via automation Jun 20, 2023
@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Patch coverage: 77.77% and project coverage change: -0.19 ⚠️

Comparison is base (2b31810) 70.79% compared to head (ee12d1f) 70.61%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2068      +/-   ##
==========================================
- Coverage   70.79%   70.61%   -0.19%     
==========================================
  Files         305      305              
  Lines       61819    61819              
==========================================
- Hits        43764    43652     -112     
- Misses      18055    18167     +112     
Flag Coverage Δ
addons 92.88% <ø> (ø)
netcore 73.35% <77.77%> (-0.17%) ⬇️
netfx 69.25% <66.66%> (-0.17%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../Microsoft/Data/Common/DbConnectionStringCommon.cs 64.51% <ø> (ø)
...rc/Microsoft/Data/SqlClient/SqlConnectionString.cs 72.01% <0.00%> (-2.47%) ⬇️
...tcore/src/Microsoft/Data/SqlClient/SNI/SNIProxy.cs 49.46% <50.00%> (ø)
...qlClient/SqlColumnEncryptionCngProvider.Windows.cs 100.00% <100.00%> (ø)
...qlClient/SqlColumnEncryptionCspProvider.Windows.cs 99.14% <100.00%> (ø)
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs 68.23% <100.00%> (-0.35%) ⬇️
...c/Microsoft/Data/SqlTypes/SqlFileStream.Windows.cs 53.61% <100.00%> (ø)
...SqlClient/ActiveDirectoryAuthenticationProvider.cs 59.57% <100.00%> (ø)
...src/Microsoft/Data/SqlClient/SqlSecurityUtility.cs 79.02% <100.00%> (ø)

... and 16 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

SqlClient v5.2 automation moved this from In progress to Reviewer approved Jun 27, 2023
@JRahnama JRahnama merged commit 3b45ee7 into dotnet:main Jun 27, 2023
132 checks passed
SqlClient v5.2 automation moved this from Reviewer approved to Done Jun 27, 2023
kant2002 pushed a commit to kant2002/SqlClient that referenced this pull request Jun 29, 2023
@JRahnama JRahnama deleted the string-comparison branch March 8, 2024 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
➕ Code Health Changes related to source code improvements
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants