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 support for non-throwing get method like standard dict. #6

Merged
merged 1 commit into from
May 17, 2014

Conversation

b4hand
Copy link
Contributor

@b4hand b4hand commented May 16, 2014

No description provided.

@@ -29,6 +29,12 @@ cdef class BaseTrie:
def __len__(self):
return (<hattrie_t_*> self._trie).m

def get(self, bytes key, int value=-1):
Copy link
Member

Choose a reason for hiding this comment

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

I think default value shouldn't be limited to int. It is an unnecessary type check, and it even has a small performance cost.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it would be weird to allow it to be non-int since the current Trie type only allows int values.

I'd be happy to change it if you want to allow storing arbitrary types in the Trie. I actually mentioned this as a desirable feature in #7. If you go ahead and merge this PR, I'd be happy to work towards getting full non-int functionality in a separate PR; although, I may need a little help as I'm not that familiar with Cython.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that it is a bit weird to return non-int, but should we pay a performance cost to actively prevent users from doing this? We can still have -1 as a default, but allow users to use any other value (e.g. None) if they want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you just want me to drop the type declaration, that sounds fine.

@kmike
Copy link
Member

kmike commented May 16, 2014

+1 to this feature.

@b4hand
Copy link
Contributor Author

b4hand commented May 17, 2014

Ok, I dropped the int type restriction.

@kmike
Copy link
Member

kmike commented May 17, 2014

Thanks!

kmike added a commit that referenced this pull request May 17, 2014
Add support for non-throwing `get` method like standard `dict`.
@kmike kmike merged commit dec3942 into pytries:master May 17, 2014
@b4hand b4hand deleted the add_get branch May 21, 2014 20:53
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