comprehensive-rust icon indicating copy to clipboard operation
comprehensive-rust copied to clipboard

Update default-methods.md

Open freeaion opened this issue 2 years ago • 8 comments

This commit updates the example code to make NotEqual a super trait for Equals. The example code implements "trait NotEqual: Equals", which makes Equals a super trait for NotEqual.

freeaion avatar May 24 '23 15:05 freeaion

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

google-cla[bot] avatar May 24 '23 15:05 google-cla[bot]

@edliaw, you introduced these examples in #643 — can you help me review this?

mgeisler avatar May 24 '23 16:05 mgeisler

Hm, it seems a little convoluted like this, and self.equal will not be available to NotEqual. Maybe instead just fix the wording to be "Make Equal a super trait of NotEqual".

edliaw avatar May 24 '23 17:05 edliaw

The current example is as follows;

  • Move method not_equal to a new trait NotEqual.
  • Make NotEqual a super trait for Equal.

trait NotEqual: Equals { fn not_equal(&self, other: &Self) -> bool { !self.equal(other) } }

I think it's better to update both example and phrase because the context for the example is to cover the blanket implementation where any types implementing Equal will have the impl of NotEqual. By making NotEqual a super trait of Equal, we can have the same implementation. If I misunderstood the context for the example, please let me know. Thank you.

freeaion avatar May 24 '23 19:05 freeaion

That does make sense. I think we'd need to leave the implementation of not_equal in Equals and remove the default implementation in NotEqual to make it work, because self.equal will not be available to NotEqual if it isn't inheriting it from Equals.

edliaw avatar May 26 '23 16:05 edliaw

Thanks a lot @edliaw for reviewing this and thanks @freeaion for pointing out that this isn't clear. Did the two of you reach a consensus?

mgeisler avatar May 31 '23 13:05 mgeisler

Hello @edliaw and @mgeisler, it's not clear on what to change in my commit for PR. (github's PR is not as good as critique in google, which makes me hard to accomodate what @edliaw was recommending.) Please let me know what needs to be changed to get LGTM. Thank you.

freeaion avatar May 31 '23 15:05 freeaion

I edited my wording in the comment above. If you try to compile your change, it should complain that "no method named equal found for NotEqual", because it is no longer inheriting self.equal from Equals. My suggestions should enable it to compile.

Hm, doing it this way is still a little convoluted though, because now everything that impls Equals needs to impl NotEqual first (and therefore not_equal, so having a default impl of not_equal doesn't really make sense).

edliaw avatar May 31 '23 16:05 edliaw

Hello @edliaw, as you pointed out, super trait cannot use a method in its sub trait, which results in a compilation error. (Oops.. :) ) I think the example doesn't allow us to show default impl for super/sub traits because it has deps btn them. I think the only way to resolve this issue is to revise the wording. Let me cancel this PR and initiate another one w/ the re-wording. github's PR is too inconvenient compared to CL/critique and/or gerrit. Thank you @edliaw.

freeaion avatar Jun 01 '23 17:06 freeaion