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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Fix reflection for PathBuf and OsString #6776

Closed
wants to merge 1 commit into from
Closed

[Merged by Bors] - Fix reflection for PathBuf and OsString #6776

wants to merge 1 commit into from

Conversation

Shatur
Copy link
Contributor

@Shatur Shatur commented Nov 27, 2022 •
edited
Loading

Objective

  • PathBuf and OsString not reflected correctly.

Solution

  • Add missing registrations.
  • Add FromReflect impls.
  • Always implement Reflect for OsString just skip Serialize and Deserialize for unsupported platforms.

Changelog

Fixed

  • Fix reflection for PathBuf and OsString.

@Shatur Shatur added this to the 0.9.1 milestone Nov 27, 2022
alice-i-cecile
alice-i-cecile approved these changes Nov 27, 2022
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Is there a good way to add a test that this continues to work?

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Reflection Runtime information about types labels Nov 27, 2022
@Shatur
Copy link
Contributor Author

Shatur commented Nov 27, 2022 •
edited
Loading

Is there a good way to add a test that this continues to work?

We need to somehow make sure that all types on which impl_reflect_value is called have registration in bevy_core.
But having a test that basically a copy-paste of all values from impl_reflect_value won't make things better, it will be just a duplication.

Shatur added a commit to projectharmonia/project_harmonia that referenced this pull request Nov 27, 2022
@Shatur
Add registration for PathBuf
bfc989c 
Will be fixed in bevyengine/bevy#6776
@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 27, 2022
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Nov 27, 2022
@Shatur
Fix reflection for PathBuf and OsString (#6776)
5230729 
# Objective

- `PathBuf` and `OsString` not reflected correctly.

## Solution

- Add missing registrations.
- Add FromReflect impls.
- Always implement `Reflect` for `OsString` just skip `Serialize` and `Deserialize` for unsupported platforms.

---

## Changelog

## Fixed

- Fix reflection for `PathBuf` and `OsString`.
@bors
Copy link
Contributor

bors bot commented Nov 27, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Fix reflection for PathBuf and OsString [Merged by Bors] - Fix reflection for PathBuf and OsString Nov 27, 2022
@bors bors bot closed this Nov 27, 2022
cart pushed a commit that referenced this pull request Nov 30, 2022
@Shatur @cart
Fix reflection for PathBuf and OsString (#6776)
93dc878 
# Objective

- `PathBuf` and `OsString` not reflected correctly.

## Solution

- Add missing registrations.
- Add FromReflect impls.
- Always implement `Reflect` for `OsString` just skip `Serialize` and `Deserialize` for unsupported platforms.

---

## Changelog

## Fixed

- Fix reflection for `PathBuf` and `OsString`.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@Shatur @ItsDoot
Fix reflection for PathBuf and OsString (bevyengine#6776)
85aa933 
# Objective

- `PathBuf` and `OsString` not reflected correctly.

## Solution

- Add missing registrations.
- Add FromReflect impls.
- Always implement `Reflect` for `OsString` just skip `Serialize` and `Deserialize` for unsupported platforms.

---

## Changelog

## Fixed

- Fix reflection for `PathBuf` and `OsString`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@MrGVSV MrGVSV MrGVSV approved these changes

Assignees
No one assigned
Labels
A-Reflection Runtime information about types C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
0.9.1
Development

Successfully merging this pull request may close these issues.
3 participants
@Shatur @alice-i-cecile @MrGVSV