datrs / flat-tree

Map a binary tree to a vector.
Apache License 2.0
53 stars 10 forks source link

consider removing xyz_with_depth() from the api? #14

Closed ralphtheninja closed 5 years ago

ralphtheninja commented 6 years ago

I have an index, lets say index = 44 and I want the offset, so I call offset(44). Does it even make sense to call offset_with_depth(44, 4)?

I suspect these functions are there because rust doesn't have default values for function parameters, which is the same thing for functions in c.

Maybe all xyz_with_depth() should be local only?

Same goes for .sibling(), .parent() etc

More concretely, does a calling lib ever pass in depth explicitly?

ralphtheninja commented 6 years ago

It's completely possible that I'm missing something, please excuse my ignorance in that case :wink:

yoshuawuyts commented 6 years ago

Maybe all xyz_with_depth() should be local only?

Possibly! I haven't seen it being used yet anywhere outside of this codebase yet, so I'm thinking it might indeed need to be private.

Perhaps we should make a little more progress on hypercore to check if it's indeed all private, and if it is we patch it here?

Really good catch!

ralphtheninja commented 6 years ago

Perhaps we should make a little more progress on hypercore to check if it's indeed all private, and if it is we patch it here?

Def! :heart:

yoshuawuyts commented 6 years ago

bad news https://github.com/mafintosh/hypercore/blob/master/lib/tree-index.js#L123

ralphtheninja commented 6 years ago

Aaah ok. Maybe we can push those algorithms down to flat-tree instead? Just thinking out loud.

ralphtheninja commented 6 years ago

I'll make a list here on the functions that do require using depth:

EDIT: removed flat.index() since it always uses depth and offset parameters

ralphtheninja commented 6 years ago

@yoshuawuyts If you find other usage, please add to the list

yoshuawuyts commented 6 years ago

@ralphtheninja will do!