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

C#: Enforce globalization code quality rules #87133

Merged

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Jan 13, 2024

Implements globalization code quality rules1. Also, removed the relevant .editorconfig suppressions of these warnings, as they're now accounted for and should be warned of moving forward.

Footnotes

  1. https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/globalization-warnings

@Repiteo Repiteo requested a review from a team as a code owner January 13, 2024 00:23
@AThousandShips AThousandShips added this to the 4.x milestone Jan 13, 2024
@Repiteo Repiteo force-pushed the dotnet/enforce-globalization-rules branch from 4eb5094 to 4b27844 Compare February 17, 2024 16:43
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

Partial review.

@Repiteo Repiteo force-pushed the dotnet/enforce-globalization-rules branch 4 times, most recently from ff9b22a to 3106289 Compare February 17, 2024 17:53
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

Great work, thank you so much! I think this looks good but I would appreciate a second pair of eyes to check that I haven't missed anything.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Feb 20, 2024
@akien-mga akien-mga requested a review from paulloz February 20, 2024 09:24
@@ -1329,7 +1330,7 @@ public static bool HtmlIsValid(ReadOnlySpan<char> color)
/// <returns>A string representation of this color.</returns>
public readonly string ToString(string? format)
{
return $"({R.ToString(format)}, {G.ToString(format)}, {B.ToString(format)}, {A.ToString(format)})";
return $"({R.ToString(format, null)}, {G.ToString(format, null)}, {B.ToString(format, null)}, {A.ToString(format, null)})";
Copy link
Member

Choose a reason for hiding this comment

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

Ditto for Plane, Projection, Quaternion, Vector2, Vector2I, Vector3, Vector3I, Vector4, Vector4I.

Do we really want to use the current culture instead of invariant here? E.g. on my machine, this would represent floating points as 3,1415927, it feels wrong compared to 3.1415927. I think we should default to CultureInfo.InvariantCulture.NumberFormat instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I agree, this was to the end of keeping the previous behavior intact. Plus, we'd probably want a way to change the culture if we're changing the default, which would be outside the scope of this PR. I'm implementing something similar in a seperate PR via IFormattable (#83159), so this default could be applied there instead

Alternatively, if everyone is onboard with making it CultureInfo.InvariantCulture.NumberFormat, I'm completely fine with applying that instead. Just a matter of gauging the other contributors' thoughts

@Repiteo Repiteo force-pushed the dotnet/enforce-globalization-rules branch from 7236dfe to 000d12d Compare February 20, 2024 17:15
Copy link
Member

@paulloz paulloz left a comment

Choose a reason for hiding this comment

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

Everything is fine by me. Thanks a lot ❤️

@akien-mga akien-mga merged commit 1aab6e9 into godotengine:master Feb 20, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@Repiteo Repiteo deleted the dotnet/enforce-globalization-rules branch February 20, 2024 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants