-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Conversation
@dotnet-bot test this please |
if (comparisionType == StringComparison.CurrentCulture) | ||
{ | ||
Assert.Equal(expected, text.StartsWith(value)); | ||
} |
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.
Why have this special case? Wouldn't be sufficient to just always do:
Assert.Equal(expected, text.StartsWith(value, comparisonType);
?
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've seen this pattern done (and done it my self) in a few places for code coverage purposes (now you get String.StartsWith(String)
covered)).
Personally, I could go either way.
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.
Ah, makes sense. Ok.
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.
Yep that was the reason. Just to get it included in the testing.
LGTM. Nice to see ICU passing on this as well! Thanks very much for these additions! |
Fixed comparisonType typo |
Additional String.StartsWith tests
Greater coverage and StringComparision usage.
"Soft Hyphen" is ignored on all Windows cultures. I'll see from the CI build whether this is also holds true on Linux. Otherwise I might have to use a fixed culture and chars known to work.