feat: Specify different separators for each children group of a commutative parent #374

Maybe rename this to something like commutative_child_separator? That makes the role of this method a bit clearer imo -- "if the children can commute, here's the separator, otherwise I'll return a None"

This is very confusing imo...

I first simplified this by replacing then_some().or() with just an if-else:

if self.children_groups.is_empty() {
    Some(self.separator)
} else {
    self.children_groups
        .iter()
        .find_map(|group| {
            if group.node_types.is_superset(node_types) {
                group.separator.or(Some(self.separator))
            } else {
                None
            }
        })
}

After that I thought I could extract .or(Some(self.separator)) out of find_map since it's applied to every element anyway, but then I realized that that's actually not completely true -- apparently we:

1. check if node_types are all in the ChildrenGroup we're looking at
2. if they are, we either use the separator ChildrenGroup specifies if it does specify one, or the default one for the whole commutative group if it doesn't

So maybe it would be nice to add a comment along the lines of what I've described above (if I understood everything correctly).

Thinking about this a bit more, why do we unconditionally return Some(self.separator) in the self.children_groups.is_empty() case? Granted, that seems to have been the case before as well, but wouldn't we need that node_types actually are all part of this commutative group? Or is that an implicit assumption that this method makes?

Yes your understanding is correct, I'll add some comments to confirm.

maybe add this as an assertion in CommutativeGroup::restricted_to?

I've been wanting to turn those slices into arrays (to avoid copying later), but it was impossible back then since you can't store arrays of different sizes in the same collection (the outer slice in this case). But now that we wrap each inner slice in ChildrenGroup::new / ChildrenGroup::with_separator, we should actually be able to. And the outer slice (which has now become a vec), should be able to be turned into an array as well.

None of that should be a part of this PR of course -- I'll try to remember to look into that after we've merged this

Nope, this still is not possible -- CommutativeGroup::restricted_to_groups still operates on slices of slices, but if we were to replace ChildrenGroup methods to accept arrays, we'd need to pass slices of arrays to restricted_to_groups, which won't work (in general) since slices of differently-sized arrays are not allowed. We could of course instead introduce new, array-based alternatives to ChildrenGroup::{new,with_separator}, but that's really not worth it

Wouldn't you need to use this trimmed_sep above, when constructing the leader vecs? Or is it not relevant there for some reason?

This line is superfluous indeed since we assume that the trimmed versions of both separators are the same. I'll remove it.

just fyi: this could also be achieved by replacing the assert! below with debug_assert! -- the then empty loop will definitely get optimized away. But this works just as well of course

Ah ok, I wasn't sure… I guess it also makes it clear to the reader that the whole check is done only in debug mode.

I guess it also makes it clear to the reader that the whole check is done only in debug mode.

Yeah, also wanted to mention that. Since the two approaches are otherwise identical, let's go with the current, more readable one

consider using assert_eq! instead?

I kind of have the same question I had earlier regarding trimmed_sep, but with raw_separator now -- why is it okay that above, we get it simply as commutative_parent.separator, but then go on to the lengths of calling child_separator? Well that's probably because we don't actually use the first raw_separator anywhere -- instead we just trim it right away.

I see that there are a couple more places around the codebase where we still access commutative_parent.separator directly, but all of them, again, trim it before using. So maybe it'd be a good idea to hide the separator field (so remove its pub)? That way, we'd either need to provide the children to get the separator that they want, or be content with the trimmed separator, since that one will be the same for the parent and the children. The second option could be realized using a new CommutativeParent::trimmed_separator() method

Sounds like an excellent idea, I'll do that.

Formatting a string with {:?} uses the Debug representation, which, among others, escapes the special characters, and enclosed the whole string in double quotes. So I think that could replace both the escape_separator helper and the single quotes in the format string?

Fantastic, that's much better of course!