-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Split the bevy_ecs reflect.rs module #8834
Conversation
Everything looks good to me and the divisions make sense. The new documentation is really helpful to understand what the ReflectComponent stuff actually is, why its there, and what you use it for. I know these are intended for internal usage, however I think reflect is something where its really really helpful to the end user to understand more of how stuff works rather than less. These new docs in particular helped me a lot to understand what ReflectComponent actually is and why I'm using it. In that regard I think a portion of or a rewrite of these docs moved to the mod file so its public and hopefully in docs.rs would be really helpful for the end user who is trying to figure out how to use reflect. As it stands ReflectComponent doesn't really have any public documentation I believe. That can definitely be a follow up pr if its a good idea though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this change: cleaner and docs are good! I'm going to merge now in the interest of unblocking future work and avoiding merge conflicts.
@NoahShomette thanks for the review :) In the future, feel free to actually use the |
Of course, thank you! Wasn't sure exactly how to do it but I'll get it done right next time! |
- Cleanup the `reflect.rs` file in `bevy_ecs`, it's very large and can get difficult to navigate - Split the file into 3 modules, re-export the types in the `reflect/mod.rs` to keep a perfectly identical API. - Add **internal** architecture doc explaining how `ReflectComponent` works. Note that this doc is internal only, since `component.rs` is not exposed publicly. To review this change properly, you need to compare it to the previous version of `reflect.rs`. The diff from this PR does not help at all! What you will need to do is compare `reflect.rs` individually with each newly created file. Here is how I did it: - Adding my fork as remote `git remote add nicopap https://github.com/nicopap/bevy.git` - Checkout out the branch `git checkout nicopap/split_ecs_reflect` - Checkout the old `reflect.rs` by running `git checkout HEAD~1 -- crates/bevy_ecs/src/reflect.rs` - Compare the old with the new with `git diff --no-index crates/bevy_ecs/src/reflect.rs crates/bevy_ecs/src/reflect/component.rs` You could also concatenate everything into a single file and compare against it: - `cat crates/bevy_ecs/src/reflect/{component,resource,map_entities,mod}.rs > new_reflect.rs` - `git diff --no-index crates/bevy_ecs/src/reflect.rs new_reflect.rs`
# Objective - Cleanup the `reflect.rs` file in `bevy_ecs`, it's very large and can get difficult to navigate ## Solution - Split the file into 3 modules, re-export the types in the `reflect/mod.rs` to keep a perfectly identical API. - Add **internal** architecture doc explaining how `ReflectComponent` works. Note that this doc is internal only, since `component.rs` is not exposed publicly. ### Tips to reviewers To review this change properly, you need to compare it to the previous version of `reflect.rs`. The diff from this PR does not help at all! What you will need to do is compare `reflect.rs` individually with each newly created file. Here is how I did it: - Adding my fork as remote `git remote add nicopap https://github.com/nicopap/bevy.git` - Checkout out the branch `git checkout nicopap/split_ecs_reflect` - Checkout the old `reflect.rs` by running `git checkout HEAD~1 -- crates/bevy_ecs/src/reflect.rs` - Compare the old with the new with `git diff --no-index crates/bevy_ecs/src/reflect.rs crates/bevy_ecs/src/reflect/component.rs` You could also concatenate everything into a single file and compare against it: - `cat crates/bevy_ecs/src/reflect/{component,resource,map_entities,mod}.rs > new_reflect.rs` - `git diff --no-index crates/bevy_ecs/src/reflect.rs new_reflect.rs`
- Cleanup the `reflect.rs` file in `bevy_ecs`, it's very large and can get difficult to navigate - Split the file into 3 modules, re-export the types in the `reflect/mod.rs` to keep a perfectly identical API. - Add **internal** architecture doc explaining how `ReflectComponent` works. Note that this doc is internal only, since `component.rs` is not exposed publicly. To review this change properly, you need to compare it to the previous version of `reflect.rs`. The diff from this PR does not help at all! What you will need to do is compare `reflect.rs` individually with each newly created file. Here is how I did it: - Adding my fork as remote `git remote add nicopap https://github.com/nicopap/bevy.git` - Checkout out the branch `git checkout nicopap/split_ecs_reflect` - Checkout the old `reflect.rs` by running `git checkout HEAD~1 -- crates/bevy_ecs/src/reflect.rs` - Compare the old with the new with `git diff --no-index crates/bevy_ecs/src/reflect.rs crates/bevy_ecs/src/reflect/component.rs` You could also concatenate everything into a single file and compare against it: - `cat crates/bevy_ecs/src/reflect/{component,resource,map_entities,mod}.rs > new_reflect.rs` - `git diff --no-index crates/bevy_ecs/src/reflect.rs new_reflect.rs`
Objective
reflect.rs
file inbevy_ecs
, it's very large and can get difficult to navigateSolution
reflect/mod.rs
to keep a perfectly identical API.ReflectComponent
works. Note that this doc is internal only, sincecomponent.rs
is not exposed publicly.Tips to reviewers
To review this change properly, you need to compare it to the previous version of
reflect.rs
. The diff from this PR does not help at all! What you will need to do is comparereflect.rs
individually with each newly created file.Here is how I did it:
git remote add nicopap https://github.com/nicopap/bevy.git
git checkout nicopap/split_ecs_reflect
reflect.rs
by runninggit checkout HEAD~1 -- crates/bevy_ecs/src/reflect.rs
git diff --no-index crates/bevy_ecs/src/reflect.rs crates/bevy_ecs/src/reflect/component.rs
You could also concatenate everything into a single file and compare against it:
cat crates/bevy_ecs/src/reflect/{component,resource,map_entities,mod}.rs > new_reflect.rs
git diff --no-index crates/bevy_ecs/src/reflect.rs new_reflect.rs