-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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 Reflect and FromReflect for AssetPath #8531
Add Reflect and FromReflect for AssetPath #8531
Conversation
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
I'm in favor of the general form TBH. |
Not sure we could have a blanket impl like that. I think the main concern is that we lock ourselves into a single |
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.
Just a few comments.
Also, I agree that we could maybe reduce the code duplication (maybe a macro_rules for &'static T
/Cow<'static, T>
?), but that can be done in a separate PR.
Implement Reflect::debug for Cow<'static, Path> Style fixes
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.
Sorry two more comments I forgot to leave 😅
Style changes
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! Thanks for doing this!
My pleasure! |
@minchopaskal can you please resolve comments that you've addressed to make things easier for further reviewers to follow? |
@alice-i-cecile done. Sorry! I made the assumption that the one who leaves comments should resolve them. |
Totally fine, it's just an arbitrary cultural choice we've made :) |
Objective
Solution
Reflect
andFromReflect
forAssetPath
Reflect
andFromReflect
forCow<'static, Path>
as to satisfy the 'static lifetime requierments of bevy_reflect. Implementation is a direct copy of that forCow<'static, str>
so maybe it begs the question that was already asked in bevy_reflect: Support Cow<'static, [T]> #7429 - maybe it would be benefitial to write a general implementation forReflect
forCow<'static, T>
.