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

Deprecate TrieSet.mem() in favor of TrieSet.contains() #576

Merged
merged 3 commits into from
Dec 6, 2023
Merged

Conversation

rvanasa
Copy link
Contributor

@rvanasa rvanasa commented Jul 13, 2023

This PR deprecates TrieSet.mem() in favor of TrieSet.has() for consistency with the base library design document.

We could update the design guide if this makes more sense, although I think has would be less ambiguous than mem, which could be misunderstood as having something to do with "memory" instead of "membership" for developers familiar with wording such as "has" / "contains" / "includes".

@rvanasa rvanasa requested a review from kentosugama July 13, 2023 20:25
@kentosugama
Copy link
Contributor

So the Buffer class's membership function is called contains, and I did a quick search and it seems like there is actually no membership function in the base library named has, despite it being in the design doc.

Maybe it makes sense to call this functions contains and update the design doc to match that?

I do agree that mem is the worst option of the bunch.

@rvanasa
Copy link
Contributor Author

rvanasa commented Jul 17, 2023

I would be okay with contains (that was actually my first suggestion while talking with @matthewhammer about this before knowing about the design doc).

@crusso crusso requested review from crusso and removed request for kentosugama December 6, 2023 17:09
@crusso crusso merged commit 4a094f9 into master Dec 6, 2023
6 checks passed
@crusso crusso deleted the trie-set-has branch December 6, 2023 17:10
@rvanasa rvanasa changed the title Deprecate TrieSet.mem() in favor of TrieSet.has() Deprecate TrieSet.mem() in favor of TrieSet.contains() Dec 6, 2023
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