bevy_reflect: Function reflection terminology refactor #14813
Merged
Conversation
This file contains hidden or 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-Docs
alice-i-cecile
approved these changes
Aug 19, 2024
Member
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
I prefer the new naming! We should be sure to ship this in 0.15.
alice-i-cecile
added
S-Ready-For-Final-Review
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 22, 2024
# Objective It looks like running `compile_fail_utils::test` on an invalid path just skips the test. I'm not sure why `ui_test` doesn't fail the test, but it seems pretty easy to accidentally cause a test to be skipped (I experienced [this](#14813 (comment)) while doing some refactoring on `bevy_reflect`). ## Solution Check to make sure the given path exists before continuing on with the tests. Alternatively, we could look into seeing why this doesn't work properly upstream. But I figured this solution was simple enough just to implement directly without having to worry about updating `ui_test`. ## Testing To verify that this works as expected `cd` into `crates/bevy_reflect/compile_fail`. Then run the following: ``` cargo test --target-dir ../../../target ``` All compile fail tests should pass. Now edit the path used in `crates/bevy_reflect/compile_fail/tests/derive.rs`. For example: ```diff fn main() -> compile_fail_utils::ui_test::Result<()> { - compile_fail_utils::test("reflect_derive", "tests/reflect_derive") + compile_fail_utils::test("reflect_derive", "tests/does_not_exist") } ``` Run the tests again: ``` cargo test --target-dir ../../../target ``` Verify the test fails with an error like: ``` Error: path does not exist: "tests/does_not_exist" ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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
One of the changes in #14704 made
DynamicFunctioneffectively the same asDynamicClosure<'static>. This change meant that the de facto function type would likely beDynamicClosure<'static>instead of the intendedDynamicFunction, since the former is much more flexible.We could explore ways of making
DynamicFunctionimplementCopyusing some unsafe code, but it likely wouldn't be worth it. And users would likely still reach for the convenience ofDynamicClosure<'static>over the copy-ability ofDynamicFunction.The goal of this PR is to fix this confusion between the two types.
Solution
Firstly, the
DynamicFunctiontype was removed. Again, it was no different thanDynamicClosure<'static>so it wasn't a huge deal to remove.Secondly,
DynamicClosure<'env>andDynamicClosureMut<'env>were renamed toDynamicFunction<'env>andDynamicFunctionMut<'env>, respectively.Yes, we still ultimately kept the naming of
DynamicFunction, but changed its behavior to that ofDynamicClosure<'env>. We need a term to refer to both functions and closures, and "function" was the best option.Originally, I was going to go with "callable" as the replacement term to encompass both functions and closures (e.g.
DynamciCallable<'env>). However, it was suggested by @SkiFire13 that the simpler "function" term could be used instead.While "callable" is perhaps the better umbrella term—being truly ambiguous over functions and closures— "function" is more familiar, used more often, easier to discover, and is subjectively just "better-sounding".
Testing
Most changes are purely swapping type names or updating documentation, but you can verify everything still works by running the following command: