-
Notifications
You must be signed in to change notification settings - Fork 325
FIX: Use the invariant culture for capitalization when generating code #2156
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
Changes from all commits
3d05994
12d1278
72ee68e
052c5dd
09987dc
5d88031
e9f8a63
9f8740c
ee83350
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,10 +73,10 @@ public static void RegisterPlatform(BuildTarget target) | |
private static bool IsPluginInstalled() | ||
{ | ||
var registeredPackages = UnityEditor.PackageManager.PackageInfo.GetAllRegisteredPackages(); | ||
var plugInName = PlugInName + EditorUserBuildSettings.activeBuildTarget.ToString().ToLower(); | ||
var plugInName = PlugInName + EditorUserBuildSettings.activeBuildTarget.ToString(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like you fixed a separate issue here right? Or are active build targets package names ToUpper? I am confused around how this previously was supposed to work. The new code looks solid though, effectively ignoring case. |
||
foreach (var package in registeredPackages) | ||
{ | ||
if (package.name.Equals(plugInName)) | ||
if (package.name.Equals(plugInName, StringComparison.InvariantCultureIgnoreCase)) | ||
return true; | ||
} | ||
return false; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -171,26 +171,22 @@ public override string ToString() | |
|
||
public static bool operator==(InternedString a, string b) | ||
{ | ||
return string.Compare(a.m_StringLowerCase, b.ToLower(CultureInfo.InvariantCulture), | ||
StringComparison.InvariantCultureIgnoreCase) == 0; | ||
return string.Compare(a.m_StringLowerCase, b, StringComparison.InvariantCultureIgnoreCase) == 0; | ||
} | ||
|
||
public static bool operator!=(InternedString a, string b) | ||
{ | ||
return string.Compare(a.m_StringLowerCase, b.ToLower(CultureInfo.InvariantCulture), | ||
StringComparison.InvariantCultureIgnoreCase) != 0; | ||
return string.Compare(a.m_StringLowerCase, b, StringComparison.InvariantCultureIgnoreCase) != 0; | ||
} | ||
|
||
public static bool operator==(string a, InternedString b) | ||
{ | ||
return string.Compare(a.ToLower(CultureInfo.InvariantCulture), b.m_StringLowerCase, | ||
StringComparison.InvariantCultureIgnoreCase) == 0; | ||
return string.Compare(a, b.m_StringLowerCase, StringComparison.InvariantCultureIgnoreCase) == 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: One could call the other operator, e.g. return (b == a) to avoid the redundant code |
||
} | ||
|
||
public static bool operator!=(string a, InternedString b) | ||
{ | ||
return string.Compare(a.ToLower(CultureInfo.InvariantCulture), b.m_StringLowerCase, | ||
StringComparison.InvariantCultureIgnoreCase) != 0; | ||
return string.Compare(a, b.m_StringLowerCase, StringComparison.InvariantCultureIgnoreCase) != 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: One could call the other operator, e.g. return (b != a) |
||
} | ||
|
||
public static bool operator<(InternedString left, InternedString right) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to fix case issues here, but I wonder if this wasn't incorrect to begin with. Shouldn't this be using https://learn.microsoft.com/en-us/dotnet/api/system.io.path?view=net-9.0 which should take platform-specific behavior of underlying file systems into account instead? Its probably fine, but it is a bit of a warning flag in my eyes when code tries to "outsmart" the filesystem implementation. Its also interesting previous code was doing ToLower() on a constant that is already in lowercase (its not an arbitrary input). The point I am after is that on some OS you may have both a XR, Xr, xR and xr directory and they are all different. (e.g. linux)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess for paths, we should probably use https://github.com/lucasmeijer/NiceIO because it makes pathing so easily and cool. For this instance, I would agree it might have been somewhat incorrect on case-sensitive file systems - which are normally Linux and the default Mac. Won't target changing behaviour here though.