Skip to content

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

Merged
peterbae merged 1 commit intomicrosoft:devfrom
peterbae:locale
Jul 18, 2017
Merged

Adding locale fix for turkey locale issue when lowercasing an "i"#384
peterbae merged 1 commit intomicrosoft:devfrom
peterbae:locale

Conversation

@peterbae
Copy link
Copy Markdown
Contributor

@peterbae peterbae commented Jul 14, 2017 •
edited
Loading

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
Copy Markdown

msftclas commented Jul 14, 2017

@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 AfsanehR-zz and xiangyushawn and removed request for xiangyushawn July 14, 2017 16:59
@codecov-io
Copy link
Copy Markdown

codecov-io commented Jul 14, 2017

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.

nsidhaye
nsidhaye reviewed Jul 15, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@nsidhaye nsidhaye left a comment •
edited
Loading

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
Copy Markdown
Contributor Author

peterbae commented Jul 18, 2017

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.

@peterbae peterbae merged commit 0f9c812 into microsoft:dev Jul 18, 2017
@peterbae peterbae mentioned this pull request Jul 18, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AfsanehR-zz AfsanehR-zz Awaiting requested review from AfsanehR-zz

1 more reviewer

@nsidhaye nsidhaye nsidhaye left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

6.3.0

Development

Successfully merging this pull request may close these issues.

6 participants

@peterbae @msftclas @codecov-io @nsidhaye @lilgreenbird @ajlam