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

Make it so ParsedPath can be passed to GetPath #9373

Merged
merged 1 commit into from
Aug 25, 2023

Conversation

nicopap
Copy link
Contributor

@nicopap nicopap commented Aug 6, 2023

Objective

  • Unify the ParsedPath and GetPath APIs. They weirdly didn't play well together.
  • Make ParsedPath and GetPath API easier to use

Solution

  • Add the ReflectPath trait.
  • GetPath methods now accept an impl ReflectPath<'a> instead of a &'a str, this mean it also can accepts a &ParsedPath
  • Make GetPath: Reflect and use default impl for Reflect types.
  • Add GetPath and ReflectPath to the bevy_reflect prelude

Changelog

  • Add the ReflectPath trait.
  • GetPath methods now accept an impl ReflectPath<'a> instead of a &'a str, this mean it also can accept a &ParsedPath
  • Make GetPath: Reflect and use default impl for Reflect types.
  • Add GetPath and ReflectPath to the bevy_reflect prelude

Migration Guide

GetPath now requires Reflect. This reduces a lot of boilerplate on bevy's side. If you were implementing manually GetPath on your own type, please get in touch!

ParsedPath::element[_mut] isn't an inherent method of ParsedPath, you must now import ReflectPath. This is only relevant if you weren't importing the bevy prelude.

-use bevy::reflect::ParsedPath;
+use bevy::reflect::{ParsedPath, ReflectPath};

 parsed_path.element(reflect_type).unwrap()
 parsed_path.element(reflect_type).unwrap()

@nicopap nicopap added C-Feature A new feature, making something new possible C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Aug 6, 2023
@nicopap nicopap added this to the 0.12 milestone Aug 6, 2023
@alice-i-cecile alice-i-cecile removed the C-Feature A new feature, making something new possible label Aug 11, 2023
Copy link
Contributor

@soqb soqb left a comment

Choose a reason for hiding this comment

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

a straightforward improvement to simplicity and performance, without sacrificing ergonomics!

reflect_trait, FromReflect, GetField, GetTupleStructField, Reflect, ReflectDeserialize,
ReflectFromReflect, ReflectSerialize, Struct, TupleStruct,
reflect_trait, FromReflect, GetField, GetPath, GetTupleStructField, Reflect,
ReflectDeserialize, ReflectFromReflect, ReflectPath, ReflectSerialize, Struct, TupleStruct,
Copy link
Contributor

Choose a reason for hiding this comment

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

not convinced we should have ReflectPath here. all of its methods can be used through GetPath, no?

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.

This makes sense to me and cleans things up significantly.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 25, 2023
Merged via the queue into bevyengine:main with commit 7083f0d Aug 25, 2023
26 checks passed
@nicopap nicopap deleted the get-path-merge branch August 30, 2023 13:35
@cart cart mentioned this pull request Oct 13, 2023
43 tasks
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-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants