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

make remove_node return data instead of node #85

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

spearman
Copy link
Contributor

This changes remove_node to return a T value instead of a Node.

@iwburns
Copy link
Owner

iwburns commented Aug 15, 2019

I do prefer this API to the existing one (as you can probably tell by how slab_tree works).

In general, I don't think users of this library should have to deal with data/constructs that don't matter to them, but the existing API forces you to deal with creating Nodes when you want to insert, and receiving Nodes when you want to remove.

With all of that said though, this is a breaking change and if we go this route I'd like to make this type of change across the whole API surface instead of just on remove_node (wherever it makes sense). I'm not saying you need to be the person to make the other changes necessarily, but you are welcome to do so if you would like.

Do you have any thoughts / opinions here?

@spearman
Copy link
Contributor Author

The way slab_tree works does seem more ergonomic, but this is the only place so far where I wasn't able to get the behavior I wanted (returning the original value without having to clone out of the node), otherwise it's been pretty usable.

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.

2 participants