-
Notifications
You must be signed in to change notification settings - Fork 123
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 Borrow
and BorrowMut
derives
#145
base: master
Are you sure you want to change the base?
Conversation
This could also be seen as a solution to #123 as |
Any particular issues with this? Anything that would need to be changed to allow it to be merged in? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work on this! I read up a bit on the difference between Borrow
and AsRef
, these two quotes from the rust docs seem the most important:
In particular Eq, Ord and Hash must be equivalent for borrowed and owned values:
x.borrow() == y.borrow()
should give the same result asx == y
.
Borrow also requires that Hash, Eq and Ord for borrowed value are equivalent to those of the owned value. For this reason, if you want to borrow only a single field of a struct you can implement AsRef, but not Borrow.
This makes it seem like maybe Borrow
should only be allowed to be derived on single field structs (i.e. newtypes). What do you think of that? Does it still cover your usecase then?
Other than that the PR is missing some tests. You can probably copy most of the as_ref
/as_mut
tests: https://github.com/JelteF/derive_more/blob/master/tests/as_ref.rs
There's also the generics test, that you should probably add it to: https://github.com/JelteF/derive_more/blob/master/tests/generics.rs
I'm pretty busy at the moment, but I'll try to make some time for a full review of this PR.
doc/borrow_mut.md
Outdated
|
||
Generates: | ||
|
||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
``` | |
```rust |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
My usecase has actually somewhat changed and I am taking a different approach by using
I've added tests for I'm not sure if there's anyone else interested in this, but I've resolved the merge conflicts just in case. |
I can say I'd appreciate a Regarding the semantics of
I established these principles from viewing the implementors of I think a reasonable compromise, to better adhere to the spirit of With all the above said, restricting the derive to single-field structs is definitely the safest option; that's all I need personally (I don't have any examples of multi-field newtypes using |
going over old PRs
I totally agree with this. I think if someone changes this PR to add the single-field restriction then it can probably be merged quickly. |
Had to switch from using
AsRef
to usingBorrow
in a project of mine for certain reasons and I'd really like to continue using this crate for that purpose.Borrow
is identical in functionality toAsRef
, but is used in different contexts and implemented for bothT
and&T
.This literally duplicates the functionality for
AsRef
, except the duplicated instances are replaced withBorrow
. Same withAsMut
andBorrowMut
, which can be used like so:Let me know if there are any issues with this and I'll fix them. I think I got everything in the documentation included that is necessary, but I'm not 100% sure.