Skip to content

Conversation

@ueco-jb
Copy link
Contributor

@ueco-jb ueco-jb commented Jul 11, 2022

closes #1349

@ueco-jb ueco-jb requested a review from webmaster128 July 11, 2022 15:18
@ueco-jb ueco-jb self-assigned this Jul 11, 2022
Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Can consistency be enforced with

        + PartialEq
        + PartialEq<&'a Self>

in the AllImpl trait?

fn eq(&self, rhs: &&Decimal) -> bool {
self == *rhs
}
}
Copy link
Member

@webmaster128 webmaster128 Jul 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, can we implement that using forward_ref_binop! too? That should give us 3 implementations: reference left, right and both

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't seem so. forward_ref_binop! looks for an Output associated type (like when you're trying to do this for Add), but PartialEq doesn't have such a type.

Copy link
Contributor

@uint uint Aug 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also don't need an implementation for refs on both sides. That case is taken care of by deref coercion.

We're only missing one for LHS, like:

impl PartialEq<Decimal> for &Decimal {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good to know, thanks @uint. Can you add tests for all combinations and the missing implementation (maybe as PR into this PR)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. Let's get 1.1.0 wrapped up.

@webmaster128 webmaster128 added this to the 1.1.0 milestone Jul 16, 2022
@ueco-jb ueco-jb removed this from the 1.1.0 milestone Jul 31, 2022
@uint
Copy link
Contributor

uint commented Aug 8, 2022

Are you still working on this?

@ueco-jb
Copy link
Contributor Author

ueco-jb commented Aug 8, 2022

Yes.

@uint
Copy link
Contributor

uint commented Aug 15, 2022

Resolved CHANGELOG.md conflict.

@uint
Copy link
Contributor

uint commented Aug 15, 2022

@webmaster128 This last commit should force the type resolution to make those ints u64 from the beginning, rather than possibly casting them.

@webmaster128
Copy link
Member

Nice, thanks!

@uint uint merged commit c106596 into main Aug 15, 2022
@uint uint deleted the 1349-implement-partialeq-for-reference-math-types branch August 15, 2022 12:53
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.

Implement PartianEq for reference for math types

4 participants