Closed klautcomputing closed 4 months ago
Hello @klautcomputing,
Thank you for you feedback. I have been mulling over my options because there are several breaking changes that might happen in the upcoming release and I would want the migration to be as pain free as possible.
I will consider your opinion.
Cheers mate, Clement
As a developer, I happily accommodate breaking changes and spend some time rewriting my code if that means things are more consistent and better for the future.
This is also pre-v1.0
so breaking changes are normal. And given how new you lib is you might not have too many active users yet, so better sooner than later 🙂
If you want some more concrete feedback feel free to share your ideas with me/the whole internet.
I wholeheartedly agree with this. And I am more than willing to accommodate breaking changes if that means better interfaces in the long term. thanks
Noted. I'll fix this in the coming release.
As promised version 0.1.4 has been published and it resolves this issues. Thank you everyone for your contribution.
Cheers.
Oh wow, that was super quick! Thank you 👍
Hi @clementwanjau,
I got a PR from someone which uses your library and while reviewing their code I noticed a couple of things with the library and though I would share them with you. But first let me say thank you for this library, it saved a lot of time not having to reinvent the wheel! 🙂
Here is some feedback on the ergonomics of the library:
In general, any function that is called
get_thing
should get you what they promise. Most of your functions do that so I was expectingNode::get_parent
andNode::get_children
to do the same, especially becauseNode::get_node_id
explicitly states that it's getting you theid
of thenode
. I thinkget_parent
andget_children
should be renamed toget_parent_id
andget_child(ren)_ids
.Another thing which I don't think is particularly great is the constant use of
unwraps
in the lib. For example:compiles fine and then blows up because
get_height
tries to return ani32
instead of anOption<i32
. I think there's two solutions to this: 1) returnOption<T>
s wherever the library currently unwraps.2) Maybe it's possible to hide all the
unwrap
related functions with the typestate pattern? But I am not sure that's doable...I understand both options require breaking changes to your library. But I think standardizing the methods a little more/improving the ergonimics and getting rid of all the unwraps are meaningful changes.
Cheers leex