feat: Implement opt-in as_ref/as_mut support in Getters/MutGetters macros #120
Conversation
|
Thanks for your contribution! For now, take a look at the CI errors. I will do a more in depth review when I get the chance. |
|
@jbaublitz Thanks for your reply. I've fixed the lint errors and rebased to the latest HEAD branch. Looking forward to your review! |
There was a problem hiding this comment.
I'm currently making my way through the PR and so far it looks reasonable. I need to familiarize myself a bit more with the string arguments to Getters and Setters only because I feel like as I've been approving and merging PRs, there's been a bit of a split between what gets implemented as an extension of get vs. what gets implemented as its own derive macro such as CloneGetters and CopyGetters. It may be worth putting up an issue to gather feedback about this and take a more disciplined approach to these sorts of issues in v0.2.0.
There was a problem hiding this comment.
I left a few code level comments that I'm happy to discuss with you. In particular the visibility parsing currently works and I don't want to hold up this PR when this might be better addressed by #124.
As for higher level feedback, I'm concerned that the only types supported are Result and Option largely because the pattern:
type MyResult<O> = Result<O, MyError>
This PR would not be able to generate any as_ref/as_mut getters in this case. I'm okay with supporting this just for Option, Result, and type aliases for these two types as the docs seem to indicate that these types' as_ref implementations are separate from the AsRef trait. Focusing on just covering all of those cases for now is fine with me.
| let value_str = parse_named_value(attr, meta_name)?; | ||
| let vis_str = value_str.split(' ').find(|v| v.starts_with("pub"))?; | ||
|
|
||
| Some(parse_vis_str(vis_str, attr.span())) |
There was a problem hiding this comment.
This was the potential problem I was worried about with this approach. I think in terms of an API, it looks very good and I prefer this approach ergonomically but I'm worried about being able to reliably parse things moving forward if other visibility modifiers are added in. It seems a bit fragile for future compatibility.
| }; | ||
| value_str.split(' ').any(|v| v == "as_ref") |
There was a problem hiding this comment.
We're splitting the string twice for each of these invocations. I think it would make sense to split is above line 276 and pass the split string as an argument to any parser that needs it.
Implementation of #119
in this PR:
as_ref/'as_mutto make theOption/Resultcan returnOption<&T>/Result<&T, &E>instead of&Option<T>/&Result<T, E>.Maintainer edit:
I believe this will also close #78