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

Be more strict when trying to convert types in IsSameOrEqualTo #1202

Merged
merged 1 commit into from Dec 12, 2019

Conversation

@jnyrup
Copy link
Collaborator

jnyrup commented Dec 8, 2019

When comparing whether two objects are equal, a conversion should be precision preserving.

Two examples of comparisons that currently unexpectedly passes:

  • 10 is not equal to true even though 10 is convertible to true as true is not convertible to 10.
  • 0.02 is not equal to 0, as that is due to truncation when converting the double into an int.

This fixes #1010
This fixes #1183

Copy link
Member

dennisdoomen left a comment

Cool. I was planning to fix this thing in the same way this weekend.

@jnyrup jnyrup force-pushed the jnyrup:SameOrEqual branch from 9af4024 to e0631d7 Dec 9, 2019
@jnyrup

This comment has been minimized.

Copy link
Collaborator Author

jnyrup commented Dec 9, 2019

I came to think that this might break (undocumented?) runtime behavior for some.

With the current code, the second test of When_comparing_a_custom_convertible_to_a_basic_type_it_should_fail unexpectedly passes, but the first fails as expected.
This is due to CustomConvertible being convertible into int.
With the changes in this PR int would also have to be convertible into CustomConvertible.

@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Dec 10, 2019

When_comparing_a_custom_convertible_to_a_basic_type_it_should_fail

Can't find that one.

@eNeRGy164

This comment has been minimized.

Copy link

eNeRGy164 commented Dec 10, 2019

When_comparing_a_custom_convertible_to_a_basic_type_it_should_fail

Can't find that one.

jnyrup@e0631d7#diff-9b95f746d5165c406e037ab57c731715R54

@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Dec 10, 2019

Duh. Wasn't looking in the PR itself.

@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Dec 10, 2019

With the changes in this PR int would also have to be convertible into CustomConvertible.

Maybe we should be much more restrictive in what we consider equal. In other words, list the types for which we apply the rules.

When comparing whether two objects are equal, a conversion should be
precision preserving.
Two examples of comparisons that currently unexpectedly passes:
* `10` is not equal to `true` even though `10` is convertible to `true`
as `true` is not convertible to `10`.
* `0.02` is not equal to `0`, as that is due to truncation when
converting the `double` into an `int`.
@jnyrup jnyrup force-pushed the jnyrup:SameOrEqual branch from e0631d7 to 8472390 Dec 11, 2019
@jnyrup

This comment has been minimized.

Copy link
Collaborator Author

jnyrup commented Dec 11, 2019

I've updated the PR to use a white-list of numeric types, for which we will try conversion between.

@jnyrup jnyrup merged commit f2a5158 into fluentassertions:master Dec 12, 2019
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@jnyrup jnyrup deleted the jnyrup:SameOrEqual branch Dec 12, 2019
jnyrup added a commit to jnyrup/fluentassertions that referenced this pull request Dec 28, 2019
Before fluentassertions#1202 we had an asymmetry where:
* `When_comparing_an_enum_and_a_numeric_for_equality_it_should_not_throw` passed, but
* `When_comparing_a_numeric_and_an_enum_for_equality_it_should_not_throw` failed.

After fluentassertions#1202 both of them failed.

This PR makes them both pass to minimize the change of behavior while fixing the asymmetry.
fluentassertions#1204 has a discussion of how to compare enums, integers and strings.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.