-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add numerical ordering option for string comparison operations #109861
Changes from all commits
87a8d03
2c51664
28b74a4
45f0b4e
48b3c6e
3d29499
8034de5
215c1d6
74150f7
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 |
---|---|---|
|
@@ -9186,6 +9186,7 @@ public enum CompareOptions | |
IgnoreSymbols = 4, | ||
IgnoreKanaType = 8, | ||
IgnoreWidth = 16, | ||
NumericOrdering = 32, | ||
OrdinalIgnoreCase = 268435456, | ||
StringSort = 536870912, | ||
Ordinal = 1073741824, | ||
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. nit: should we change this to use hex numbers for the sake of the readability? 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. This is how the GenAPI tool generates it and the guidance I've heard is to make as few diffs from that tool as possible. 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. Yes, but still can make diffs :-) 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. The ref assembly isn’t really designed for readability, it’s designed to be correct and automatically generated Deviations and manual diffs just cause later downstream pain and hinder the ability to rerun the tool, as people have to fight against it The better option would be to submit a bug or better a patch such that the output produced by the tool for flags enabled enums is “better” such as using hex or logical shifts to represent the bits instead ( 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. I've created an issue: dotnet/sdk#44999. GenAPI seems to be in a kind of limbo state where there is a new Roslyn based version we want to switch to (tracked by dotnet/sdk#31088) and we don't want to maintain the current CCI-based one (https://github.com/dotnet/arcade/blob/main/src/Microsoft.DotNet.GenAPI/README.md). |
||
|
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.
will be good to add a remarks to this one giving more information like this option can be used in comparisons but not for search (IndexOf/StartsWith/EndsWith). Will be good to hint the behavior difference too when ICU is used against NLS. And last tell this option cannot be combined with the ordinal operations,
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.
The comment was getting long so I just included just the part about indexing. Mentioning search might be confusing because people might consider IndexOf (which isn't supported) as search and GetHashCode (which is supported) as not. I think it's easier to just say that NumericOrdering works in all cases except for indexing.
I prefer to keep the combination behavior with Ordinal and OrdinalIgnoreCase on those members since this is really a property of theirs instead of numeric ordering. This is also consistent with the other options as well which don't mention Ordinal even though they can't combine with them either.
I think the ICU and NLS differences should probably go in docs rather than in doc comments since NLS usage is not going to be high. There are already docs about NLS/ICU differences that we can append to (https://learn.microsoft.com/en-us/dotnet/core/extensions/globalization-icu#behavioral-differences).
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.
Well, if you prefer that, it will be better to edit the docs of the indexing APIs and add the remark there. I am trying to make it easy for the API users to understand when this new enum value is not allowed. I guess users can be puzzled if they get exceptions and do not understand what is wrong.
You don't have to block the PR on that, but it will be good to open a doc issue/PR to add the info as needed.
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 like that idea. I just checked and we already list the valid options so I will update those docs once this goes in:
Compare
IndexOf