Skip to content
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

[Merged by Bors] - impl Reflect for &'static Path #6755

Closed
wants to merge 3 commits into from

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Nov 25, 2022

Objective

Fixes #6739

Solution

Implement the required traits. They cannot be implemented for Path directly, since it is a dynamically-sized type.

@MrGVSV MrGVSV added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types labels Nov 25, 2022
@@ -1019,6 +1100,27 @@ impl FromReflect for Cow<'static, str> {
}
}

impl Typed for &'static Path {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these impls separated from the Reflect impl?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No particular reason, I just copied how the other impls in the file are organized.

Copy link
Member

@MrGVSV MrGVSV Nov 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh lol looks like I someone messed up in placing the Option<T> impls 🤦

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I don't think there is anything else to register apart from those. So this is probably fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might make more sense to support serialization for Cow<'static, Path> instead.

@james7132 james7132 added this to the 0.9.1 milestone Nov 25, 2022
@alice-i-cecile
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Nov 25, 2022
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Nov 25, 2022
@bors
Copy link
Contributor

bors bot commented Nov 25, 2022

try

Build failed:

@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Nov 25, 2022
# Objective

Fixes #6739 

## Solution

Implement the required traits. They cannot be implemented for `Path` directly, since it is a dynamically-sized type.
@bors bors bot changed the title impl Reflect for &'static Path [Merged by Bors] - impl Reflect for &'static Path Nov 26, 2022
@bors bors bot closed this Nov 26, 2022
cart pushed a commit that referenced this pull request Nov 30, 2022
# Objective

Fixes #6739 

## Solution

Implement the required traits. They cannot be implemented for `Path` directly, since it is a dynamically-sized type.
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.
@JoJoJet JoJoJet deleted the reflect-path branch January 13, 2023 02:30
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
Labels
A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

std::path::Path doesn't implement Reflect in 0.9
4 participants