sublist: Added template to the stub file#610
Conversation
| Unequal, | ||
| } | ||
|
|
||
| pub fn sublist<T: Debug>(first_list: &[T], second_list: &[T]) -> Comparison { |
There was a problem hiding this comment.
it's a little unfortunate - normally it would not be necessary to restrict this to Debug but it is done for the sake of being able to reference it in the unimplemented message
We explicitly made a different choice in c5f7a43 . We chose to use underscore prefix for this
I would find using an underscore prefix acceptable. I'll take some time to think about whether it is preferable over adding the Debug constraint. Input appreciated.
There was a problem hiding this comment.
Some points why I think Debug could be preferable:
- The lists from the test suite contain only primitive types, each of them implements
Debug. If the test suite is extended with some custom types (unlikely) it should not be that hard to derive the trait for the new type. - It could help students in debugging their solutions in case they want to print the input arguments
- It increases learning for the students who may have forgotten about the trait bounds
There was a problem hiding this comment.
The axiom that @petertseng and I are working from is that when possible, one should implement one's library with the minimally restrictive trait bounds which enable it to work. In other words, a library should attempt to function for the largest possible range of types. This is because, while a more restrictive trait bound may be fine in the moment, you don't know the circumstances in which the library will be used in the future.
You're right that for the purposes of this exercise, Debug would be fine. However, one of our design goals in the Rust track is to model good engineering principles. Say you write a library which bounds its generics on Debug. A user Alice wants to use your library, but she hasn't written the type she's using; it comes from a third-party library which she doesn't control. By misfortune, it doesn't implement Debug. Alice is out of luck.
I've temporarily added a Debug trait bound for debugging while writing my own code. Sometimes println debugging is a useful technique! However, I always try to remove it before committing; it's not good practice, and it's not what we should be modeling.
There was a problem hiding this comment.
Agreed. I will remove the trait bound then.
Although I suspect in your example Alice could still implement a trait for third-party type, since that's what the trait system is about, but I think this argument could wait for now.
There was a problem hiding this comment.
You can't implement a foreign trait for a foreign type, though. What you could do is write a wrapper, implement Debug for that and painstakingly re-implement the rest of the behaviour to forward to the internal type. The contained type would still be opaque to Debug, but at least it would work.
|
Should the type restriction in |
|
I can confirm two things:
Must a valid solution to this exercise do so? If so, is it better to include it or leave it off? |
|
@petertseng, as per @coriolinus comment the amount of trait bounds should be minimized and I think this means we should leave off the |
|
I believe that for this exercise, |
|
I will add the There are only two moments of concern for me:
For now I think the answers could be:
|
We could add only the type for the very first test and let the student add the generics bound to make the other tests compile. The issue of the difference between PartialEq and Eq can be forced by one of the later tests by using a non-Eq type.
I really don't see how |
Just from top of my head: computing a hash of every object and comparing hash values |
|
True, that approximates it. It's not quite the same algorithm though, because it's probabilistic and can have false positives. |
coriolinus
left a comment
There was a problem hiding this comment.
This PR looks good to me. There is obviously some open discussion (#631) about what trait bounds are appropriate, but this subjectively seems to get the balance right.
I intend to merge within the next 48 hours. I have no objection if another maintainer wants to merge sooner.
Contributes to #551