-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Finish the work on try_apply
#12646
Finish the work on try_apply
#12646
Conversation
Current TODO:
|
8c712a6
to
4e255ee
Compare
The code is mostly finished. I'm just not confident about |
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.
Looks good! Just a few comments/suggestions.
I also added suggestions to move some of the macro code to use fully-qualified syntax (hopefully with the bulk-apply for PR suggestions, that can take some of the work off your plate).
Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
04d06a7
to
8c213ff
Compare
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.
all this LGTM. just two little things before i approve.
Objective
Finish the
try_apply
implementation started in #6770 by @feyokorenhof.Supersedes and closes #6770. Closes #6182
Solution
Add
try_apply
toReflect
and implement it in all the places that implementReflect
.Changelog
Added
try_apply
toReflect
.