Added optional deref implementation for Getters#19
Added optional deref implementation for Getters#19Razican wants to merge 4 commits intojbaublitz:masterfrom Razican:deref
Conversation
|
I added a new commit, fixing the typos in the comments and adding tests and an example in the README. |
| /// Doc comments are supported! | ||
| /// Multiline, even. | ||
| #[get] #[set] #[get_mut] | ||
| #[get] #[deref] #[set] #[get_mut] |
There was a problem hiding this comment.
Can I ask why you chose this option over something like #[get ="deref"] or #[get = "pub deref"]?
There was a problem hiding this comment.
It was mostly to be coherent with the current implementation. The string associated with the attribute was the "visibility" of the function, so it was a bit strange to add the deref part there. I can change it, of course, but it was strange for me.
Also, if we used the deref in that place, would we allow #[get="deref pub"]? or only #[get="pub deref"]. What about complex visibility modifiers such as #[get="pub(crate)"]?
It can be done, but i think it's a bit more cumbersome to define.
There was a problem hiding this comment.
Yeah. I'm not entirely sure I favor either way. One bother for me is that #[get_mut] #[deref] seems odd. It makes #[deref] into a no-op. Also #[deref] alone would be a no-op.
That said, I could see a legitimate use case under #21 .
There was a problem hiding this comment.
So, I thought about this more. I think we should take a conservative approach.
The goal of this library is to reduce the boilerplate associated with trivial getters and setters in a minimal fashion to support library evolution in a practical manner.
Notably: Field Access is not ABI stable across struct versions. Having Getters and Setters helps with stability.
Thankfully visibility modifiers are fairly easily tokenizable. We can create a simple set from the tokens present, and if deref (which is not a valid visibility modifer) is present we can do this deref implementation.
How does that sound to you?
I think ideal would be something like #[get(vis = "pub", deref)] but at least when I tried a when I first wrote this it didn't seem possible.
|
@Boscop Could you PTAL and add your insights? :) |
|
@Boscop Ping :) |
|
What's up with this PR? Getting this would make getset a lot more useful for my use-case (which involves making getters for a lot of structs that have numeric fields) |
|
@roblabla It needs more reviewers! If you're willing to help review code please give a review. :) |
|
Would it be possible to update and merge this PR? Also, would it be possible to automate it so that types with the Of course, this change would imply a release that increments the minor semver version to prevent breaking crates accidentally (0.1.0 in this case.) |
|
I don't think it's possible to automate? It's impossible to check the traits a type implements from a proc macro. At best, it might be possible to hardcode a list of certain types (like |
|
@Jojolepro It seems like this PR is quite stale and not ready for merge. If you'd like to reflect some of the feedback (notably #19 (comment)) or come up with a new design, I'd be very happy to review and accept it. Unfortuantely, I'm rather busy and don't have time to go re-implement this feature which I do not need. |
|
@roblabla I think there is too much magic in that approach. :( |
|
Superseded by #29 , thanks for spending some time helping us find the ideal solution for now! :) |
Hello, I've seen this being tackled by multiple implementations and I wanted to add this option too. It basically enables the
$[deref]attribute for getters.