[Merged by Bors] - impl Reflect for &'static Path
#6755
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MrGVSV
added
C-Usability
MrGVSV
reviewed
Nov 25, 2022
| @@ -1019,6 +1100,27 @@ impl FromReflect for Cow<'static, str> { | |||
| } | |||
| } | |||
|
|
|||
| impl Typed for &'static Path { | |||
There was a problem hiding this comment.
Why are these impls separated from the Reflect impl?
There was a problem hiding this comment.
No particular reason, I just copied how the other impls in the file are organized.
There was a problem hiding this comment.
Oh lol looks like I someone messed up in placing the Option<T> impls 🤦
There was a problem hiding this comment.
It's fine to leave it for now. I'll follow up with a quick cleanup PR!
| impl GetTypeRegistration for &'static Path { | ||
| fn get_type_registration() -> TypeRegistration { | ||
| let mut registration = TypeRegistration::of::<Self>(); | ||
| registration.insert::<ReflectFromPtr>(FromType::<Self>::from_type()); |
There was a problem hiding this comment.
Are there other registrations we should include here? I think we could do ReflectSerialize but I'm not sure ReflectDeserialize is possible for a reference (?) so maybe we shouldn't include either? 🤔
There was a problem hiding this comment.
Hm, I don't think there is anything else to register apart from those. So this is probably fine.
There was a problem hiding this comment.
It might make more sense to support serialization for Cow<'static, Path> instead.
MrGVSV
approved these changes
Nov 25, 2022
|
bors try |
alice-i-cecile
approved these changes
Nov 25, 2022
alice-i-cecile
added
the
S-Ready-For-Final-Review
tryBuild failed: |
|
bors r+ |
bors
bot
changed the title
Reflect for &'static PathReflect for &'static Path
bors bot
pushed a commit
that referenced
this pull request
Dec 3, 2022
# Objective > Followup to [this](#6755 (comment)) comment Rearrange the impls in the `impls/std.rs` file. The issue was that I had accidentally misplaced the impl for `Option<T>` and put it between the `Cow<'static, str>` impls. This is just a slight annoyance and readability issue. ## Solution Move the `Option<T>` and `&'static Path` impls around to be more readable.
ickshonpe
pushed a commit
to ickshonpe/bevy
that referenced
this pull request
Dec 6, 2022
# Objective > Followup to [this](bevyengine#6755 (comment)) comment Rearrange the impls in the `impls/std.rs` file. The issue was that I had accidentally misplaced the impl for `Option<T>` and put it between the `Cow<'static, str>` impls. This is just a slight annoyance and readability issue. ## Solution Move the `Option<T>` and `&'static Path` impls around to be more readable.
alradish
pushed a commit
to alradish/bevy
that referenced
this pull request
Jan 22, 2023
# Objective > Followup to [this](bevyengine#6755 (comment)) comment Rearrange the impls in the `impls/std.rs` file. The issue was that I had accidentally misplaced the impl for `Option<T>` and put it between the `Cow<'static, str>` impls. This is just a slight annoyance and readability issue. ## Solution Move the `Option<T>` and `&'static Path` impls around to be more readable.
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
# Objective Fixes bevyengine#6739 ## Solution Implement the required traits. They cannot be implemented for `Path` directly, since it is a dynamically-sized type.
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
# Objective > Followup to [this](bevyengine#6755 (comment)) comment Rearrange the impls in the `impls/std.rs` file. The issue was that I had accidentally misplaced the impl for `Option<T>` and put it between the `Cow<'static, str>` impls. This is just a slight annoyance and readability issue. ## Solution Move the `Option<T>` and `&'static Path` impls around to be more readable.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Fixes #6739
Solution
Implement the required traits. They cannot be implemented for
Pathdirectly, since it is a dynamically-sized type.