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

Document _prefix index matching for queries #89

Merged
merged 1 commit into from
Feb 1, 2022

Conversation

dnephin
Copy link
Contributor

@dnephin dnephin commented Feb 2, 2021

This behaviour is documented on an unexported function, but it doesn't show up in the godoc. I've attempted to move it to all the exported functions that call getIndexValue

Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

Do you mind rebasing / resolving conflicts? I think this would be a nice addition otherwise.


On a related note I've been thinking about adding a simple memdb.Prefix(string) function which would avoid the need for "special strings" and make navigation and search through codebase with prefixed indexes easier. However it doesn't look very appealing from the perspective of character length vs just simple _prefix appended to the name. :|

I just feel that this should be somehow more prominent part of the API. I didn't even realize it existed before reading through Nomad/Consul codebases and realizing _prefix actually has a special meaning.

@dnephin dnephin force-pushed the dnephin/document-get-prefix-index branch from 328b380 to 55e6bf9 Compare January 31, 2022 22:49
@dnephin
Copy link
Contributor Author

dnephin commented Jan 31, 2022

Thanks for having a look! I've rebased this PR.

I agree with you about exposing it in some way that makes it more obvious. I was also thinking that memdb.Prefix(string) would be nice.

@dnephin dnephin merged commit 82790a6 into master Feb 1, 2022
@dnephin dnephin deleted the dnephin/document-get-prefix-index branch February 1, 2022 16:04
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