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

Add ArgumentConstraintManagerExtensions.Contains overload that takes a StringComparison #1681

Closed
blairconrad opened this issue Oct 26, 2019 · 7 comments · Fixed by #1747
Closed
Assignees
Milestone

Comments

@blairconrad
Copy link
Member

blairconrad commented Oct 26, 2019

Currently we force the comparison to use the current culture.

@thomaslevesque
Copy link
Member

on platforms that support it

We could also emulate it on platforms that don't. Something like this maybe?

@blairconrad blairconrad changed the title Add ArgumentConstraintManagerExtensions.Contains overload that takes a StringComparison, on platforms that support it Add ArgumentConstraintManagerExtensions.Contains overload that takes a StringComparison Nov 5, 2019
@thomaslevesque
Copy link
Member

I think we have a bug in the existing Contains method... Or at least, an inconsistency between the platforms that have the overload with StringComparison and the ones that don't.

The code is like this:

#if FEATURE_STRING_CONTAINS_COMPARISONTYPE
            return manager.NullCheckedMatches(x => x.Contains(value, StringComparison.CurrentCulture), x => x.Write("string that contains ").WriteArgumentValue(value));
#else
            return manager.NullCheckedMatches(x => x.Contains(value), x => x.Write("string that contains ").WriteArgumentValue(value));
#endif

But the comparison used by the Contains overload without StringComparison is Ordinal, not CurrentCulture, so the behavior is not the same in the two versions.

This issue was introduced in https://github.com/FakeItEasy/FakeItEasy/pull/1666/files#diff-3f9aad371d3919486c2640596c21b7dcR80

@blairconrad
Copy link
Member Author

Oh, interesting. And good point. Yeah, we should probably fix that. Better to revert to the ordinal behaviour, I think. And maybe this increases the priority of #1681?

@thomaslevesque
Copy link
Member

Yes (in fact, this is #1681, I didn't open a new issue). I'm working on it

@thomaslevesque thomaslevesque self-assigned this Jan 10, 2020
@thomaslevesque thomaslevesque changed the title Add ArgumentConstraintManagerExtensions.Contains overload that takes a StringComparison [DO NOT MERGE] Add ArgumentConstraintManagerExtensions.Contains overload that takes a StringComparison Jan 12, 2020
@thomaslevesque thomaslevesque changed the title [DO NOT MERGE] Add ArgumentConstraintManagerExtensions.Contains overload that takes a StringComparison Add ArgumentConstraintManagerExtensions.Contains overload that takes a StringComparison Jan 12, 2020
@thomaslevesque
Copy link
Member

@thomaslevesque thomaslevesque changed the title Add ArgumentConstraintManagerExtensions.Contains overload that takes a StringComparison [DO NOT MERGE] Add ArgumentConstraintManagerExtensions.Contains overload that takes a StringComparison 5 hours ago

Oops, I confused the PR with the issue...

@blairconrad blairconrad added this to the vNext milestone Jan 13, 2020
@afakebot
Copy link

This change has been released as part of FakeItEasy 6.0.0.

@thomaslevesque
Copy link
Member

We should probably add overloads for StartsWith and EndsWith as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants