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

Adding locale fix for turkey locale issue when lowercasing an "i" #384

Merged
merged 1 commit into from
Jul 18, 2017

Conversation

peterbae
Copy link
Contributor

@peterbae peterbae commented Jul 14, 2017

using toLowerCase() without any locale makes it implicitly toLowerCase(Locale.getDefault()). This is fine in most counties but in Turkey, the default locale converts uppercase "I" to a dotless "i", which is not the same as the lowercase "i" in English. Passing Locale.ENGLISH (or Locale.US) in toLowerCase() solves this problem. I have made these changes to all the applicable places in the codebase.

Fixes issue #381

@msftclas
Copy link

@peterbae,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

@peterbae peterbae requested review from xiangyushawn and AfsanehR-zz and removed request for xiangyushawn July 14, 2017 16:59
@codecov-io
Copy link

Codecov Report

Merging #384 into dev will increase coverage by 0.02%.
The diff coverage is 33.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev     #384      +/-   ##
============================================
+ Coverage     40.16%   40.18%   +0.02%     
- Complexity     1892     1894       +2     
============================================
  Files           107      107              
  Lines         24482    24481       -1     
  Branches       4038     4038              
============================================
+ Hits           9833     9838       +5     
+ Misses        12815    12800      -15     
- Partials       1834     1843       +9
Flag Coverage Δ Complexity Δ
#JDBC41 40.07% <33.33%> (+0.02%) 1886 <0> (+3) ⬆️
#JDBC42 40.02% <33.33%> (ø) 1886 <0> (+1) ⬆️
Impacted Files Coverage Δ Complexity Δ
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java 49.35% <0%> (-0.15%) 212 <0> (+2)
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java 45.14% <0%> (+0.22%) 0 <0> (ø) ⬇️
...QLServerColumnEncryptionAzureKeyVaultProvider.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...c/main/java/com/microsoft/sqlserver/jdbc/Util.java 47.51% <100%> (ø) 63 <0> (ø) ⬇️
.../microsoft/sqlserver/jdbc/SQLServerConnection.java 45.06% <66.66%> (ø) 267 <0> (ø) ⬇️
src/main/java/microsoft/sql/DateTimeOffset.java 37.14% <0%> (-2.86%) 8% <0%> (-2%)
...om/microsoft/sqlserver/jdbc/ReaderInputStream.java 46.06% <0%> (-2.25%) 17% <0%> (ø)
...om/microsoft/sqlserver/jdbc/SimpleInputStream.java 47.4% <0%> (-1.49%) 9% <0%> (-1%)
...m/microsoft/sqlserver/jdbc/SQLServerStatement.java 59.06% <0%> (-0.31%) 129% <0%> (-1%)
...ncurrentlinkedhashmap/ConcurrentLinkedHashMap.java 42.2% <0%> (-0.22%) 46% <0%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3bd68b5...ab0dfa3. Read the comment docs.

Copy link
Contributor

@nsidhaye nsidhaye left a comment

Choose a reason for hiding this comment

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

Few Suggestions:

  1. Instead of using direct "".toLowerCase you can add new wrapper function(s) in StringUtil class. If locale is null then will use current default implementations.

@ajlam ajlam modified the milestones: 6.2.2, 6.3.0 Jul 17, 2017
@peterbae
Copy link
Contributor Author

Hi Nikhil, thanks for the suggestion. I think providing a wrapper function for toLowerCase() and adding more locale flexibility is a good idea, but currently we only want to explicitly use English as our locale to avoid case conversion problems, and I think this change solves that problem. I'll come back to that feature later when it becomes necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants