-
Notifications
You must be signed in to change notification settings - Fork 253
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
Added StringComparison to StringAssert Contains(), EndsWith() & StartsWith() #691
Added StringComparison to StringAssert Contains(), EndsWith() & StartsWith() #691
Conversation
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 unintended) breaking change
@HeroMaxPower are you going to address the PR comments or should I? |
Yes please, i haven't had time in my personal life to look at these. 👍 |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@304NotModified Would you review again please? I had to pass empty array to disambiguate the calls (before we passed null). The compiler will pass empty array for us, so we should not be doing anything wrong. Unless the user passed StringComparison as the actual first param, then they now would get different overload than before. But that is unlikely scenario. |
@304NotModified any updates? |
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.
Looks good to me! Thanks for the reminder!
@nohwnd i think this is ready to be merged 👍 |
@Haplois could you review and merge please? |
Description:
Added StringComparison to StringAssert Contains, EndsWith & StartsWith
Added tests to test functionality
Added API comment on StringComparison Enum
Updated APIs to default to StringComparison.Ordinal
Probably needs API documentation updates if this gets merged.
Resolves #154