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.

Sign up for GitHub

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

Jump to bottom

Allow borrowed keys for lookups #25

Merged
merged 1 commit into from
Jan 24, 2020
Merged

Allow borrowed keys for lookups #25

merged 1 commit into from
Jan 24, 2020

Conversation

cuviper
Copy link
Collaborator

@cuviper cuviper commented Jan 24, 2020

This matches the standard library's approach to key lookups, allowing
any Q where K: Borrow<Q>. This lets you use unowned values to query
the maps, e.g. a map with K = String can be accessed with Q = str.

@cuviper
Allow borrowed keys for lookups
474f525 
This matches the standard library's approach to key lookups, allowing
any `Q where K: Borrow<Q>`. This lets you use unowned values to query
the maps, e.g. a map with `K = String` can be accessed with `Q = str`.
@cuviper
Copy link
Collaborator Author

cuviper commented Jan 24, 2020

I think this is technically a breaking change. Current uses of &K will still work since Borrow is reflexive, but the added type parameter on the methods would affect anything that explicitly specifies those parameters already.

jonhoo
jonhoo reviewed Jan 24, 2020
View reviewed changes
src/node.rs Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jan 24, 2020

Codecov Report

Merging #25 into master will decrease coverage by 1.27%.
The diff coverage is 100%.

Impacted Files Coverage Δ
src/lib.rs 82.08% <100%> (-1.86%) ⬇️
src/node.rs 45.45% <100%> (ø) ⬆️

@jonhoo
Copy link
Owner

jonhoo commented Jan 24, 2020

Beautiful! It is a breaking change, but since we're at 0.0.1 I don't worry too much about that :)

@jonhoo jonhoo merged commit 800b3b6 into jonhoo:master Jan 24, 2020
@jonhoo jonhoo mentioned this pull request Jan 24, 2020
Merged
@cuviper
Copy link
Collaborator Author

cuviper commented Jan 24, 2020

Thanks!

Just as a rule of thumb going forward -- any time any API takes a &K parameter, it can probably use &Q instead. Same for &T when sets are added.

This was referenced Jan 24, 2020
Merged
Merged
@jonhoo
Copy link
Owner

jonhoo commented Jan 24, 2020

Yup! This is why #10 mentioned

Make sure to look at the bounds that std::collections::HashMap uses!

:p

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonhoo jonhoo jonhoo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cuviper @jonhoo