Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add label collection methods, clean up API a bit #19

Merged
merged 2 commits into from
Jun 6, 2017

Conversation

travisbrown-stripe
Copy link
Contributor

This PR rebases @jhoon-stripe's additions in #15 and addresses @thomas-stripe's comments, and also cleans up a few things (starting along the lines of Tom's suggestions).

There's no rush on this, and it includes several (superficial) breaking changes, so if people don't think it's worthwhile I'm happy to scrap it—I just wanted to open the PR before I forget.

The main breaking changes have to do with consistency of the fold* and reduce* signatures. Currently there's some variation in whether the label or children come first in functions that require both—e.g. in NodeRef#reduce, TreeOps#fold, etc., the label comes first, but in NodeRef#fold and FullBinaryTreeOps#foldNode the children come first:

def reduce[X](f: (A, X, X) => X)(g: B => X): X              // FullBinaryTree#NodeRef
def fold[A](node: Node)(f: (Label, Iterable[Node]) => A): A // TreeOps

def fold[R](f: (NodeRef, NodeRef, A) => R, g: B => R): R             // FullBinaryTree#NodeRef
def foldNode[A](node: Node)(f: (Node, Node, BL) => A, g: LL => A): A // FullBinaryTreeOps

I've changed all signatures across the board to put the label first, since that seemed to be the more common case.

There's also some variation in whether folds or reductions that take two function arguments give each its own parameter list (reduce above) or combine them in a single list (all the others). In the case of two function arguments I don't think there's a strong case that one approach or the other is more idiomatic, and putting them together is slightly nicer for type inference (and I think more common here), so I've done that throughout.

Another breaking change is related to @thomas-stripe's comment here suggesting a new reduceFullBinaryTree method. Since reduceNode is already specific to FullBinaryTreeOps, there's not really any reason it needs the Iterable in its signature, so I've rewritten it to be more or less Tom's suggested reduceFullBinaryTree (but following my new conventions above).

@thomas-stripe also notes that collectLabels could go on TreeOps, and this is also the case for collectLabelsF, so I've put both of them there.

Unfortunately we can't make the breaking changes with a deprecation cycle, since a lot of the signatures are the same after erasure, but I'd be happy to take responsibility for writing up release notes and adding the commas / making the minor rearrangements that our code using bonsai will need.

@thomas-stripe
Copy link
Contributor

This looks good - I'm not too concerned about breaking compatibility, since I don't believe this lib is really used by anyone but us currently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants