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

Fix #5: Store complete key in leaf nodes. #8

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

nickpalmer
Copy link

No description provided.

@@ -647,6 +649,15 @@ func (n *ArtNode) copyMeta(other *ArtNode) {
n.prefixLen = other.prefixLen
}

// Key() returns the key used when the node was inserted
func (n *ArtNode) Key() interface{} {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this return []byte instead of interface{} ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

@kellydunn
Copy link
Owner

Heya!

Thanks for your PR, I appreciate you taking the time to do it! I have a few comments for you on your work; I think there are some improvements that can be made such that we don't change the method signature of NewArtNode and so that *ArtNode.Key() returns the same type as we originally inserted into the key.

Let me know your thoughts! Would love to have this in the lib :)

-Kelly

@nickpalmer
Copy link
Author

Let me know if you have further comments.

return a
}
return b
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this got caught as a new file (art_node.go~), is it perhaps a temporary file that got commited by accident?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. Sorry. Was editing on a different machine than usual.

@kellydunn
Copy link
Owner

Thanks again! @nickpalmer :)

I left a few more comments, it looks like there was a temp file that was commited, and I had a small change suggestion to how insertHelper was being invoked.

Aside from that, I think I'd like to see a test that might assert that a newly created LeafNode does in fact have the Key it was intended to have originally. That would be awesome!

(Thanks again for all your hard work!)

* Was editing on a different machine.
@nickpalmer
Copy link
Author

nickpalmer commented May 24, 2016

I already added a test that asserts the newly inserted key matches the expected key. See art_tree_test.go:230-233

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