Update Custom Set tests to follow standard, update API #114
Conversation
|
Before merge, I'd like:
|
|
PR submitted to reduce the size of the test suite. |
|
since the PR to slim down the test suite is in limbo, I'm wondering if you have any thoughts on my new API or implementation? |
|
|
||
| pub fn len(&self) -> usize { | ||
| self.elements.len() | ||
| impl<T: PartialEq + Ord + Clone> CustomSet<T> { |
There was a problem hiding this comment.
out of curiosity: since https://doc.rust-lang.org/std/cmp/trait.Ord.html tells me Ord depends on PartialEq, does having PartialEq here make a difference?
There was a problem hiding this comment.
PartialEq is not necessary. Thanks for noticing that.
|
May be possible to use more iterator methods rather than creating a vector and conditionally pushing things onto it, but I admit this may be my style bias talking. There is a desire that the example implementation be idiomatic; my current shortcoming is I'm not heavily involved with Rust enough to know whether it is idiomatic to always use iterator methods where possible instead of conditionally pushing onto a mutable vec. |
|
I definitely try to favor iterators. Let me poke at the code and see what I can do. |
IanWhitney
force-pushed
the
update_custom_set
branch
from
7578a25 to
7a36e0c
Compare
|
If this looks good I'd like to merge it. Depending on the outcome of exercism/problem-specifications#232, there might be another PR that tweaks this exercise further. |
| CustomSet::new(self.collection | ||
| .iter() | ||
| .filter(|c| !other.contains(c)) | ||
| .map(|c| c.clone()) |
There was a problem hiding this comment.
Is this a time where https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.cloned would be useful?
|
I have some small comments, otherwise I feel OK. Check whether all the Just so it's clear what I reviewed, I didn't examine the entire test suite for matching with the exercism common tests so I'm pretty much assuming that if everything's passing it's good. This will mean that if there's a test that's missing I won't know. |
|
The tests follow the current standard json file, though not in the exact same order. I did leave out the parts of the current standard that implement mixed-type sets because I didn't think that was appropriate for a Rust implementation. But all the other tests are there. All 64 of them. |
|
All right, my last question is just about whether |
|
Looks like there are two conversations that are folded away in outdated diffs I think we're ok with saying 'No' to the 2nd question. However, I would like to pursue the idea of structuring 2-3 exercises so that they introduce lifetimes. That is an unique concept to Rust (afaik) and it is one people struggle with. As for question 1, now that I see the similarity to HashSet, that change makes sense to me. |
As discussed here exercism#114 (diff)
|
👍 |
|
Ok. I'll rebase down to a single commit & merge. |
I'm addressing two things in this commit. Bringing Tests in line with the Standard ==== Our suite of tests for Custom Set did not follow the [standard json file in x-common](https://github.com/exercism/x-common/blob/d6cd42db16fce5090bb486821a36d0fb1bfb7df2/custom-set.json) They tests have different function names, don't cover symmetric_difference and introduced some functions (`is_superset`) that aren't in the standard test file. I have not implemented the entire standard test suite as it covers sets of [mixed types](https://github.com/exercism/x-common/blob/d6cd42db16fce5090bb486821a36d0fb1bfb7df2/custom-set.json#L475) which I don't think is even possible in Rust. Vectors have to be homogeneous. Tuples can mix types, but can't change size. Mixed type sets may be possible, but implementing that seems way beyond the scope of this exercise. I think [this suite of tests is too long](exercism/discussions#10). But until I get a PR in to shorten the standard test suite, I figured I might as well follow it. Updating the API ==== I had a couple of problems with the current approach. First, the tests hide the actual API in a helper method. This was probably done to reduce duplication, but I find that it obscures the function under test. So I removed those. The tests also pushed towards a bunch of implementation that I found unnecessary, specifically the whole [Iter implementation](https://github.com/exercism/xrust/blob/04b665e989b578c4e46ff100b1b0a00d5df9bb0d/exercises/custom-set/example.rs#L118). There might be ways to get the current test suite passing without implementing Iter, but I got pretty stuck. My new implementation does not require this implementation, since equality comparisons are now handled by via PartialEq.
IanWhitney
force-pushed
the
update_custom_set
branch
from
1dd217b to
ad99a31
Compare
IanWhitney
changed the title
I'm addressing two things in this commit.
Bringing Tests in line with the Standard
Our suite of tests for Custom Set did not follow the standard json file in x-common They tests have different function names, don't cover symmetric_difference and introduced some functions (
is_superset) that aren't in the standard test file.I have not implemented the entire standard test suite as it covers sets of mixed types which I don't think is even possible in Rust. Vectors have to be homogeneous. Tuples can mix types, but can't change size. Mixed type sets may be possible, but implementing that seems way beyond the scope
of this exercise.
I think this suite of tests is too long. But until I get a PR in to shorten the standard test suite, I figured I might as well follow it.
Updating the API
I had a couple of problems with the current approach. First, the tests hide the actual API in a helper method. This was probably done to reduce duplication, but I find that it obscures the function under test. So I removed those helpers.
The tests also pushed towards a bunch of implementation that I found unnecessary, specifically the whole Iter implementation. There might be ways to get the current test suite passing without implementing Iter, but I got pretty stuck. My new implementation does not require this implementation, since equality comparisons are now handled by implementing PartialEq.