Pov exercise

[devcyjung]

Linked from this forum post:

new-rust-practice-exercise-suggestion-pov

Disregard the last PR #2076 please, I confused example.rs with lib.rs there

[github-actions]

Hello. Thanks for opening a PR on Exercism 🙂

We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in.

You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch.

If you're interested in learning more about this auto-responder, please read this blog post.

---

Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it.

[senekor]

Thanks, this looks like a great start! This is a quick initial pass, I'll do a more in-depth review later.

Btw. you don't have a close the PR and open another one if something isn't right. Just add your improvements to the branch and push again, the PR will be updated automatically.

[senekor]

I'm not yet sure about the difficulty. I'm not asking to change it yet, it's just a note for me so I don't forget to think it through.

[senekor]

Please don't mark things as resolved without a comment, I posted this so I don't forget about it. I'm not sure if the difficulty should be 10 or maybe reduced to 7, I'll have to compare with the other exercises.

[senekor]

Here's the list of difficult exercises on the rust track:

• parallel-letter-frequency
• macros
• poker
• forth
• ocr-numbers
• react
• circular buffer
• rectangles
• xorcism
• dominoes
• doubly-linked-list

With the exception of ocr-numbers and rectangles, all of these are more difficult than pov in my opinion. I would actually consider downgrading ocr-numbers and rectangles to medium, I don't see what's difficult about them.

A comparable exercise is simple-linked-list, it also involves Option<Box<T>>. But it's clearly easier, it doesn't involve any weird manipulation of the tree.

Looking over all the other medium exercises, pov is clearly more difficult than most of them. Few if any of the medium exercises actually require dealing with Rust's ownership system in a meaningful sense like pov. So, while I still think it's on the lower end of difficult, I'm okay with keeping it there.

[senekor]

The exercise is not primarily about generics, so what do you think about dropping them and using something like this for the label:

// In production code, this would be a type parameter on `Tree`.
pub type Label = &'static str;

I'm not 100% sure, maybe it's dumbing it down too much. But right now I'm not seeing the value of making the user implement this with generics.

[devcyjung]

I initially started with non-generic Tree, but along the way I realized that making this non-generic doesn't really make the exercise any simpler. So I added generic. It also has expected trait bound of PartialEq and Clone, so users can expect where their solution should be directed to.

Also with the non-generic Tree I spent a lot of time should I make this return type impl AsRef or just String, or Cow<'_, str>, etc

[senekor]

Please don't mark threads as resolved unless it's actually resolved, i.e. there is no reason for me or you to look at it again.

Also with the non-generic Tree I spent a lot of time should I make this return type impl AsRef or just String, or Cow<'_, str>, etc

What return type are you referring to? I would think you could use Label or &'static str in any position where you're now using T.

I think generics do make the exercise more complicated. It's not guaranteed that everyone who solves this exercise is already experienced with them. That's why I asked for the specific value they add. Unless generics add value, I would prefer to remove them.

[senekor]

Answering from this comment here on the topic of removing / keeping generics:

Also designing the data structure itself is a big part of this exercise. That's why I like the generic idea. Data structure is the best use case of generics.

I don't think generics must necessarily be a big part of the exercise just because it's about a data structure. I think the essence of this exercise is how to handle Option<Box<T>> with mutation. The choice between Vec and HashSet is neither important nor interesting.

If this is the ultimate challenge for data structure design, I think we should go for a proper generic support

I do not think this is the ultimate challenge for data structure design. I'm much more interested in a focused exercise about handling Option<Box<T>> with mutation.

The Trait bound that I chose didn't come from own my implementation. If that were the case I would have added Hash so I can use HashMap for trail.

I see this as another argument against generics. With the current bounds, users cannot use a HashSet for their implementation. They have to further restrict the bounds with Eq + Hash.

There's no reason not to go generic imo.

I think the fact that the trait bounds are either too restrictive or too loose for any given implementation is a pretty good argument against generics. In addition to that, I maintain that it's needless complexity that distracts from the learning purpose of the exercise.

---

As commented elsewhere, I now see an argument in favor of generics. Please use the following bounds: T: Debug + Eq + Hash. This will allow us to add a test case with a type that doesn't implement Clone ensuring users don't cheat by cloning nodes.

[devcyjung]

I fixed all the newlines but if there's still the same issue with file ending then it's probably auto deleted somewhere. I'll need to find a pre commit hook or something like that

[senekor]

nit: add trailing newline

[senekor]

Please don't mark threads as resolved unless it's actually resolved, i.e. there is no reason for me or you to look at it again.

Also with the non-generic Tree I spent a lot of time should I make this return type impl AsRef or just String, or Cow<'_, str>, etc

What return type are you referring to? I would think you could use Label or &'static str in any position where you're now using T.

I think generics do make the exercise more complicated. It's not guaranteed that everyone who solves this exercise is already experienced with them. That's why I asked for the specific value they add. Unless generics add value, I would prefer to remove them.

[senekor]

I don't think this is worth adding the regex dependency for. Here's a simple solution:

fn camel_to_snake(input: String) -> String {
    let mut chars: Vec<_> = input.into();
    let mut i = 0;
    while i + 1 < chars.len() {
        let (left, right) = (chars[i], chars[i + 1]);
        if right.is_ascii_uppercase() {
            chars[i + 1] = right.to_ascii_lowercase();
            if left.is_ascii_alphabetic() {
                chars.insert(i + 1, b'_');
            }
        }
        i += 1;
    }
    chars.try_into().unwrap()
}

fn main() {
    let input = "fromPov";
    let output = camel_to_snake(input.into());
    assert_eq!(output, "from_pov");
}

It doesn't have to be perfect. Our input is limited to what people put in problem-specifications and the generator never runs on the user's side.

[devcyjung]

Ok I'll make no regex version

[senekor]

Suggested change

	todo!("Implement a function that creates a new Tree with given {label:?}");
	todo!("Create a new Tree with the given {label:?}");

[senekor]

Suggested change

	todo!("Implement getter for label.");
	todo!("Return the tree's label.");

[senekor]

This looks like a builder method, but it isn't. I think this is more idiomatic:

pub fn with_children(self, children: Vec<Self>) -> Self { todo!() }

Users of this API can then construct the tree with Tree::new(label).with_children(vec![]).

The standard implementation would be to make self mutable and overwrite the children. Vec doesn't allocate when empty, so there's no perf downside.

---

Now hat I think about it, I think there's an even better option:

pub fn with_child(self, child: Self) -> Self { todo!() }

Then we just call that method once per child. Users will have to append the child to their data structure of choice. If we already give users a Vec in the constructor, we push them to use a Vec internally too, although a HashSet might also be a good choice.

[devcyjung]

Ok in that case the testcases will be a lot longer, but if you are ok with that I'll do so

[devcyjung]

with_children naming came from the Vec::with_capacity

But either way I'm good with that.

[senekor]

Yeah, making the tests longer is not that big of a deal. I like the last option because it doesn't prime the user to use a specific data structure. Another way to do that would be to have a parameter children: impl Iterator<Item = Self>. That can be collected into any data structure, vec or hash set. Either way is fine by me.

[devcyjung]

I think I'm leaning to with_children(self, Vec) idea

First, with_child is unclear for student. We are not dealing with a linked list or binary tree. It's a n-ary tree and that should be clear from the methods.
Second impl Iterator is basically I: Iterator if it's parameter. This also heavily deviates from the main idea of the practice. with_children and new are just setups so that we get the the main meat which is pov_from and path_to

I want to avoid generic method if possible. Generic itself is not alien to students. All the basic of basic types are generic. Vec, Box, HashMap, Result. etc.
What's alien to most students is generic method

Such as fn <I,V,F>....... and I agree this is unnecessary complication for this exercise. This fits in exercise like List Op, but not here.

[senekor]

In that case, go with one of these two options:

fn with_child(self, child: Self) -> Self {}
fn add_child(&mut self, child: Self) {}

I'm against with_children(self, Vec) because it gently pushes people toward a specific implementation.

[senekor]

I don't like that we need this function in the first place. It's only used in the util sorter function... the tests don't require it directly. What if we just tell users that they must ensure tree comparison doesn't fail due to a different order of children? They would have to options of ensuring that:

1. Use a HashSet instead of a Vec to store children.
2. Always sort children themselves.

---

This is not relevant if we can remove the function, but I think the idiomatic API here would be:

Suggested change

	pub fn children(&self) -> Vec<&Self> {
	pub fn children(&self) -> impl Iterator<Item = &Self> { todo!() }

[devcyjung]

All the language tracks that I've tried this exercise so far don't demand specific sort order. Some people may not want to use HashSet, some people might want to swap_remove on Vec to reduce push cost.

I don't think we should require specific type of data structure or order. That's not really mentioned in any part of the problem spec. The problem is about reparenting between parent and child. Also designing the data structure itself is a big part of this exercise. That's why I like the generic idea. Data structure is the best use case of generics. Also in most language tracks this is a Generic problem, unless the language has none, or very poor generic support

The main idea of this exercise is Make a tree, and allow some manipulation. So it is all about data structure design. And this is one of the harder exercises on the platform. It's Hard in most tracks. Certainly harder with Rust. If this is the ultimate challenge for data structure design, I think we should go for a proper generic support, so students can learn how to make a useful and flexible data structure in Rust with this exercise.

The Trait bound that I chose didn't come from own my implementation. If that were the case I would have added Hash so I can use HashMap for trail. I didn't. Both come from the requirement. PartialEq is obviously necessary for label matching, and Clone is necessary because we want them to make immutable solution. It's in the canonical json spec comments that language tracks can choose to enforce immutability, and in Rust making mutable solution is arguably more complex anyways. I chose the minimal Trait bound required by the specs.

Obviously some students can look at the test cases and notice T is just String, and use all the traits that String has. That's not desirable, but if that's how they want to solve this exercise, they can. If generic is really hard for some students they can just do impl Tree<String> instead. There's no reason not to go generic imo. Generic is hard when the method also has its type parameter and where clauses. I made none of those method signatures.

[senekor]

You mixed up the topic of the thread here. This thread is about whether or not we need users to implement the function children as a publicly exposed API. The comments you made about generics should be in a different comment thread. I'll repost them there in quoted blocks so we don't get things mixed up. So my answer here is only about the children API. Please keep threads on-topic in the future.

All the language tracks that I've tried this exercise so far don't demand specific sort order.

We don't have to implement things exactly the same way as other language tracks. Rust has two properties that not all other langues have:

1. The implementor of a type provides its own comparison function.
2. Encapsulation is idiomatic, publicly exposing internals for the sake of testing is discouraged.

These two properties make it generally unidiomatic to extend foreign types with custom sorting logic. IMO this justifies adding some additional requirements to the exercise. Asking users to make sure their data type compares equal independent of the order of the children also fits well with the instructions themselves, which describe the trees as undirected. The operation of reparenting wouldn't make sense in a tree where the order of children matters.

[senekor]

Even if we keep the generics, why are all labels converted to String? Wouldn't &'static str work perfectly fine? The tests look rather cluttered with all the .to_string().

[devcyjung]

In that case the Tree will be Tree<'a str>
I wanted to test against the most typical type of generic structure, which owns T, so Tree<String>

[senekor]

Generic data structures don't care if the thing they're holding is owned or not, there's no reason to test against one instead of the other. Let's use &str to reduce the noise in the tests.

[senekor]

In that case, go with one of these two options:

fn with_child(self, child: Self) -> Self {}
fn add_child(&mut self, child: Self) {}

I'm against with_children(self, Vec) because it gently pushes people toward a specific implementation.

[senekor]

Answering from this comment here on the topic of removing / keeping generics:

Also designing the data structure itself is a big part of this exercise. That's why I like the generic idea. Data structure is the best use case of generics.

I don't think generics must necessarily be a big part of the exercise just because it's about a data structure. I think the essence of this exercise is how to handle Option<Box<T>> with mutation. The choice between Vec and HashSet is neither important nor interesting.

If this is the ultimate challenge for data structure design, I think we should go for a proper generic support

I do not think this is the ultimate challenge for data structure design. I'm much more interested in a focused exercise about handling Option<Box<T>> with mutation.

The Trait bound that I chose didn't come from own my implementation. If that were the case I would have added Hash so I can use HashMap for trail.

I see this as another argument against generics. With the current bounds, users cannot use a HashSet for their implementation. They have to further restrict the bounds with Eq + Hash.

There's no reason not to go generic imo.

I think the fact that the trait bounds are either too restrictive or too loose for any given implementation is a pretty good argument against generics. In addition to that, I maintain that it's needless complexity that distracts from the learning purpose of the exercise.

---

As commented elsewhere, I now see an argument in favor of generics. Please use the following bounds: T: Debug + Eq + Hash. This will allow us to add a test case with a type that doesn't implement Clone ensuring users don't cheat by cloning nodes.

[senekor]

Here's the list of difficult exercises on the rust track:

• parallel-letter-frequency
• macros
• poker
• forth
• ocr-numbers
• react
• circular buffer
• rectangles
• xorcism
• dominoes
• doubly-linked-list

With the exception of ocr-numbers and rectangles, all of these are more difficult than pov in my opinion. I would actually consider downgrading ocr-numbers and rectangles to medium, I don't see what's difficult about them.

A comparable exercise is simple-linked-list, it also involves Option<Box<T>>. But it's clearly easier, it doesn't involve any weird manipulation of the tree.

Looking over all the other medium exercises, pov is clearly more difficult than most of them. Few if any of the medium exercises actually require dealing with Rust's ownership system in a meaningful sense like pov. So, while I still think it's on the lower end of difficult, I'm okay with keeping it there.

[senekor]

You mixed up the topic of the thread here. This thread is about whether or not we need users to implement the function children as a publicly exposed API. The comments you made about generics should be in a different comment thread. I'll repost them there in quoted blocks so we don't get things mixed up. So my answer here is only about the children API. Please keep threads on-topic in the future.

All the language tracks that I've tried this exercise so far don't demand specific sort order.

We don't have to implement things exactly the same way as other language tracks. Rust has two properties that not all other langues have:

1. The implementor of a type provides its own comparison function.
2. Encapsulation is idiomatic, publicly exposing internals for the sake of testing is discouraged.

These two properties make it generally unidiomatic to extend foreign types with custom sorting logic. IMO this justifies adding some additional requirements to the exercise. Asking users to make sure their data type compares equal independent of the order of the children also fits well with the instructions themselves, which describe the trees as undirected. The operation of reparenting wouldn't make sense in a tree where the order of children matters.

[senekor]

quoting from another comment:

Clone is necessary because we want them to make immutable solution. It's in the canonical json spec comments that language tracks can choose to enforce immutability, and in Rust making mutable solution is arguably more complex anyways.

I hadn't noticed that yet, but I think the solution should definitely use a mutable approach. Without that, marking the exercise as difficult makes no sense. Users should be encouraged to reuse allocations whenever reasonably possible. Rust is a performance-oriented language. That doesn't mean performance has to be the focus of everything, but we shouldn't design our APIs in a way that forces users into an inefficient implementation.

I propose:

fn pov_from(tree: &mut Option<Box<Tree>>, from: &T) { todo!() }

If desired, this can be split in two types Tree and Node, where the tree only holds a Option<Box<Node>> and the node is where the action happens in terms of the data structure.

Now I actually see an argument in favor of generics... With generics, we could add a special test case with a type that doesn't implement Clone, to make sure users don't add the Clone bound themselves to cheat.

[devcyjung]

@senekor Can you please explain why tree is Option? How about &mut Box<Tree>, and instead return a bool to mark absence of match

[senekor]

Hm, I don't know. I think last time I had to implement something like this, using Option was the way to go. But I'm not sure, could be different in this case. If &mut Box<Tree> works fine, then I'm happy with that too.

[devcyjung]

@senekor ok I'll implement as close to what you suggested, do you expect it will be merged soon by then? It will take a day or two for me I think since the API is totally different

[senekor]

I cannot say in advance when this will be merged.

[senekor]

At this point, please don't mark any conversations as resolved, ever. I need the resolved state to keep track of what has been completed and what hasn't. I will mark conversations as resolved when I do a review and find that I won't need to review it again in the future.

[devcyjung]

Apologies on the delay! I'm working hard on the solution, will commit today

[devcyjung]

Sorry again, I'm failing 1 case again and again, I'll make sure to push by today

[senekor]

No worries, there is no pressure. This is an open source project, people have lives and stuff to do, nobody is expecting you to finish something by a specific deadline! Take it easy and simply rerequest a review once you're done! 🙂

[senekor]

You can remove the prefix underscore here. This is why CI is failing. The compiler actually won't complain about unused code, because it knows that fields of type PhantomData are intended to never be read.