always use the 'en-US' culture for .ToUpper() when creating the new property name
#6404
Conversation
| // Make uppercase the first letter | ||
| return char.ToUpper(baseName[0]).ToString() + baseName.Substring(1); | ||
| // Make the first character upper case using the "en-US" culture | ||
| var firstCharacter = _englishUSCulture.TextInfo.ToUpper(baseName[0]); |
There was a problem hiding this comment.
The portable APIs don't have char.ToUpper(char, CultureInfo) but according to the reference source for that method, I can get the same result by using TextInfo.
There was a problem hiding this comment.
Can you add a comment and point to the issue on github around why this was done?
|
Tagging @dotnet/roslyn-ide for review. |
|
Does this work properly if, say, I have accented characters? Or characters in a language like farsi/Arabic? |
|
Why en-US and not invariant? |
|
@CyrusNajmabadi: Good catch. I've added tests with Arabic, Spanish (leading accented character), and Greek identifiers. |
|
This code doesn't handle surrogates/logical characters split into multiple actual chars, but the code has always had this bug and we should treat this as a separate bug which I'll file. |
|
@tmat I'll reverse the question, why invariant? |
|
Because that's what one is supposed to use when not caring about a specific culture. |
|
We do care about the culture here. If we had a way for the developer to say in VS "this is the culture/locale that I'm editing right now" then we would use that (it would never be invariant). There aren't plans right now to add one, so this is a compromise. Based on that, we're choose the most likely language that a user is editing in, in this case, en-US. For IntelliSense, we did a slightly different approach; we search using the current user's culture, and en-US, but here we have to choose one, so we went with en-US, as that was the least surprising. |
|
I see. Sounds good then. |
| @@ -340,6 +341,7 @@ protected IMethodSymbol CreateGet(string originalFieldName, IFieldSymbol field, | |||
| } | |||
|
|
|||
| private static readonly char[] s_underscoreCharArray = new[] { '_' }; | |||
| private static CultureInfo _englishUSCulture = new CultureInfo("en-US"); | |||
There was a problem hiding this comment.
We're allocating one of these for Completion, too. Worth sharing?
There was a problem hiding this comment.
As soon as your PR with this gets merged I'll rebase and share.
|
👍 |
1 similar comment
|
👍 |
… property name includes tests with turkish, arabic, spanish, and greek identifiers
brettfo
force-pushed
the
encapsulate-field-english-culture
branch
from
bd71abf
to
ead7af7
Compare
|
Rebased to take @rchande's changes and share the |
| return char.ToUpper(baseName[0]).ToString() + baseName.Substring(1); | ||
| // Make the first character upper case using the "en-US" culture. See discussion at | ||
| // https://github.com/dotnet/roslyn/issues/5524. | ||
| var firstCharacter = CompletionRules.EnUSCultureInfo.TextInfo.ToUpper(baseName[0]); |
There was a problem hiding this comment.
Should we extract that static out to some utility class somewhere? I'm sure @jasonmalinowski and couplingchecker would be weeping right now.
There was a problem hiding this comment.
I considered adding Features\Core\Portable\Common\CultureSomethingHelper.cs but ultimately decided against it due to that class only containing one item, but I could be very easily convinced otherwise.
There was a problem hiding this comment.
There was a problem hiding this comment.
@brettfo: thanks. I'll limit my weeping until that gets fixed.
|
Approved pending successful tests (if related to Dustin's change, then OK). |
|
test prtest/win/dbg/eta please |
always use the 'en-US' culture for `.ToUpper()` when creating the new property name
As per an internal discussion, it was decided that it was best to use the "en-US" culture when calling
char.ToUpper()to create the new property's name. C# and VB share the same code path so only C# tests were added.Fixes #5524.