Skip to content

Conversation

@kimsey0
Copy link
Contributor

@kimsey0 kimsey0 commented Nov 8, 2019

IMPORTANT

  • The code complies with the Coding Guidelines for C#.
  • The changes are covered by a new or existing set of unit tests which follow the Arrange-Act-Assert syntax such as is used in this example.
  • If the contribution affects the documentation, please include your changes to documentation.md in this pull request so the documentation will appear on the website.

Fixes #1096

@eNeRGy164
Copy link
Contributor

Shouldn't StartWith and EndsWithcalls in other places be adjusted? To make sure FA is consistent on all OS/Cultures.
F.e. in the following files:

  • CallerIdentifier.cs
  • Execution/LateBoundTestFramework.cs
  • Primitives/StringAssertions.cs
  • Types/TypeSelector.cs

@jnyrup
Copy link
Member

jnyrup commented Nov 10, 2019

Agree.
We should also be explicit when using the following string methods, as not specifying a StringComparison defaults to StringComparison.CurrentCulture.

  • StartsWith
  • EndsWith
  • IndexOf
  • LastIndexOf
  • Compare

@kimsey0
Copy link
Contributor Author

kimsey0 commented Nov 10, 2019

I did originally do a full search and replace from StringComparison.CurrentCulture to StringComparison.Ordinal, but since there are no existing tests for culture-sensitive behavior and I'm not sure I can figure out the potential impact of changing it in each of the places mentioned by @eNeRGy164, I ended up only committing the changes in StringAssertions.

Should I go ahead with the full search and replace plus adding StringComparison.Ordinal to all the calls mentioned by @jnyrup?

@jnyrup
Copy link
Member

jnyrup commented Nov 10, 2019

@kimsey0 It would be an appreciated contribution if you would do the replacements/additions and succeed in adding culture aware tests. That should finally(!) give Fluent Assertions consistent and expected behaviour.
If you need other cultures besides Danish to test against, we have earlier fixed problems due to German and Polish cultures.

@jnyrup
Copy link
Member

jnyrup commented Dec 12, 2019

@kimsey0 How are things working out?
We're getting close to release 5.10.0, and it would be great to have this in.
Anything's blocking you or just lack of time?
Let me know if you need any help.

@kimsey0
Copy link
Contributor Author

kimsey0 commented Dec 16, 2019

Just lack of time. How's your timeline on 5.10.0? When would you like to have this merged for it to ship with that release?

@jnyrup
Copy link
Member

jnyrup commented Dec 16, 2019

@kimsey0 We don't have a lot of progress from merging a PR to releasing a new version, so in practice in can be merged last minute.

I am hoping to release 5.10.0 this year.

@dennisdoomen
Copy link
Member

Let's move this to 6.0. Hopefully @kimsey0 is willing to do the work @jnyrup requested.

@dennisdoomen dennisdoomen changed the base branch from master to develop January 4, 2020 08:22
@jnyrup jnyrup added this to the 6.0 milestone Jan 4, 2020
@dennisdoomen
Copy link
Member

@kimsey0 are you still up for it?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

String .BeEquivalentTo() is case-sensitive on linux

4 participants