in the new exercise knapsack the fn signature have items: Vec<Item>, i do think it might be a good idea to change it to items: &[Item] and adjust probably a few tests. this would make the function more flexible, allowing it to accept both vectors and slices of Item. i wanted to check where you think this is a good idea and if you think so i can create a pr.
- Does this change help students gain fluency in the language?
- Is this a problem spec exercise? Would this change apply across tracks?
Possibly a little bit, yes.
Yes. No.
I don’t see any reasoning about this design choice documented in the PR.
@dem4ron Was your choice of Vec<Item> instead of &[Item] deliberate?
@senekor What do you think?
If this is Rust specific and not something which would apply across tracks, that docs says,
While we do have mechanisms for only some tracks to have certain test-cases, we tend to discourage it, as this forking can offer confusion to both maintainers and students.
This suggestion is not about tests. There will not be any ‘forking’ when it is accepted.
Conceptually, the function maximum_value doesn’t need to consume its input values, it only needs to read them. In such cases, I agree that accepting a slice is more idiomatic than a vector. I agree with the proposal, though it doesn’t seem very important for me.
Note that this will be a breaking change for people who have already submitted a solution. The tests will need to be updated to pass slices to the user defined function, so any solution where maximum_value accepts an owned Vec<Item> won’t compile anymore. The exercise was added very recently, so I guess it won’t be too bad.
Using AsRef<[Item]> is an option to avoid breaking changes, but I don’t think it’s idiomatic to use generics everywhere unless actually needed.
since the signature with the slice might be more idiomatic i would say it can slightly help.
if the goal of the track was to master your problem solving skills it won’t matter too much, however since exercism promise to develop fluency in your chosen programming languages it might be expected to have an idiomatic approach.
it doesn’t change tests really, it’s a matter of passing &items instead of items in tests.
I am in favor of doing this. But I have no experience with breaking changes to exercises, I hope someone can chime in on that topic.
My guess is that a lot of users don’t really go back to old exercises to update them. And those that do don’t mind a little churn. They might even be happy about it:
“Aha! They changed the signature from Vec<Item> to &[Item]. I guess the second is more idiomatic. I learned something today!”
…is what I imagine they might think.
I don’t think worrying about breaking solutions is warranted. Right now only 12 people have started the exercise, and only 4 have completed it. (source)
As far as I know a change to only the stub file does not invalidate any solutions. If the tests file isn’t touched, practically no-one will notice the change.
Oh that’s great. Didn’t know we had such detailed stats on exercises public.
The tests will need to be touched, to pass a reference to a vector instead of an owned one. But that’s really a very small number of people being affected.
i think tests will fail for those who already submitted, since their will have Vec<Item> in signature and the tests will have a slice, i tried and i have this error
7 | pub fn maximum_value(max_weight: u32, items: Vec<Item>) -> u32 {
| ^^^^^^^^^^^^^
help: consider removing the borrow
|
106 - assert_eq!(maximum_value(max_weight, &items), 80);
106 + assert_eq!(maximum_value(max_weight, items), 80);
Ah, this one: knapsack accept slices by victor-prokhorov · Pull Request #1680 · exercism/rust · GitHub
I expect it will be reopened, after which track maintainers will look at it.
Hey, sorry for the late response, I was on holiday.
I’m absolutely fine with changing Vec<Item> to &[Item]. Thanks for the suggestion!
I admit I kinda rushed through adding this while also working on a few other exercises, as my main focus was on testing and improving the exercise generator script.
@victorprokhorov I reopened the PR.
You need to update the example solution’s signature too, then it will be good to go!
that one is in knapsack/.meta/example.rs