Culture sensitivity

[patricksadowski]

Closes #138

There are still some string concatenation that do not use an invariant output format. But the main points of the issue are fixed.

[drewnoakes]

Is the default "warning" here?

[patricksadowski]

I got warnings. The docs categorizes the rules as globalization warnings.

[drewnoakes]

Were these transformations performed manually or via automation?

[patricksadowski]

Hand made with ♥. I don't know any tool for that.

[drewnoakes]

I thought perhaps the analyzer came with a codefix.

[patricksadowski]

Great idea!

[drewnoakes]

Pardon my ignorance, but how can culture influence string concatenation? For example, other cases were previously $"Foo {nameof(Bar)}", which I believe the compiler is within rights to optimise to "Foo Bar" at compile time.

[patricksadowski]

String interpolation uses the current culture. There are issues for cases like

var bar = 1.23;
$"Foo {bar}"

but (and you are right) not for cases like:

var bar = 1.23;
$"Foo {nameof(bar)}"

If you like I revert the changes for these cases with nameof.

[patricksadowski]

Finally also cases like

var bar = 1.23;
string.Concat("Foo ", bar);
"Foo " + bar;

are not taking culture into account. These cases have not been translated as mentioned in the PR description.

[drewnoakes]

Did you only fix warnings produced by the analyzer, or did you search for all string interpolation?

I don't see warnings when reverting some of these changes. Where formattable types are involved, fair enough, but many of these changes involve only strings and the result is a reduction in both readability and performance.

https://sharplab.io/#v2:D4AQDABCCMB0DiAbA9gIwIaIJYC90BctkA7AbgFgAoKmANigCYIBhKgbyoi6gGYpp6MSAFloACizF8ELAEoIAXgB8EACQAiAIIAhZhDZYAvhAAiAUQBi6ipW69+g6CIZihEAM7zlarbv3vjcysbOxA+On4RHlcnDy8VDR09NmJ0AFsAU2QAMzFPQMtrTm5irjCHSIhhABYJKRl5bm8hWAtkACc0gjFmAFdEfF72jIBJYmzkWDGAN3R2rHQpPoGhjIAaCF9ksAKrDblSCAB6I4hUDPx8DPbS+wi3YQBWGMhPRRUWts7u5cHhsYmU2Is3mi3wv1WGy2+h2pkKG08hxOEAA7h13BlbuV7rFhLQXnF3pFWh0uvgev0/qNxpMZnMFktKZDNkkYbt1BtUpkcnlZLIkac0e0MVRDEA=

I should point out that I'm also trying to work out whether the analyzer can be improved in order to provide feedback to that team.

[patricksadowski]

Visual studio helped me to find all places. I didn't get warnings. Knowing that interpolated strings use the current culture I tried to find them all. Validation was done with git grep.

I found another notation the keep the readability.

FormattableString.Invariant($"ABC {i} DEF");

Do you like that?

[drewnoakes]

@patricksadowski thanks for taking the initiative on this. Looks good. Just a few questions to help me understand.

[drewnoakes]

My original review was focussed on how the analyzer works and whether it could be improved.

Thinking about this some more I realise something deeper. #138 is about a bug in parsing where the input is known to be invariant, but parsing code relies upon the current culture.

This PR fixes that, however it also forces all formatted output to use invariant culture, which is not correct. The library should produce output strings that match the user's culture.

If this was merged as-is, all strings would appear in invariant culture (which is essentially en-US).

[patricksadowski]

This PR fixes that, however it also forces all formatted output to use invariant culture, which is not correct. The library should produce output strings that match the user's culture.

Because of your #138 (comment) I tried to fix the whole issue. I think I have another unterstanding of culture sensitivity.
If the app outputs English text then also numbers should be formatted like English numbers: The value equals 1.23 instead of The value equals 1,23 (German).
If the library supports multiple languages then text shoud be available in each supported language: The value equals 1.23 and Der Wert ist gleich 1,23

To get this done I propose to fix only the main bug of the issue. Your #138 (comment) should be done in a second PR oder issue. Is it OK for you?

[drewnoakes]

Because of your comment I tried to fix the whole issue

Yeah sorry, I hadn't thought the implications through when I wrote that. It wasn't intended as a strong statement. It may be possible to configure the analyzer to do what we want (via attributes or patterns in the .editorconfig file).

If the app outputs English text then also numbers should be formatted like English numbers

This is debatable. I'm pretty sure if this were changed then we'd see bug reports. Ideally all output strings would be localised.

To get this done I propose to fix only the main bug of the issue.

That's certainly the safest way forward here. Perhaps we add a bit more context to the TODO in the .editorconfig file too.

[drewnoakes]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[drewnoakes]

Thanks very much Patrick.