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

Split the bevy_ecs reflect.rs module #8834

Merged
merged 1 commit into from Jun 19, 2023

Conversation

nicopap
Copy link
Contributor

@nicopap nicopap commented Jun 13, 2023

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

@nicopap nicopap added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change A-Reflection Runtime information about types labels Jun 13, 2023
@NoahShomette
Copy link
Contributor

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.

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.

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.

@alice-i-cecile
Copy link
Member

@NoahShomette thanks for the review :) In the future, feel free to actually use the Approve functionality in the Files Changed tab to make it easier to track. Bevy uses a system where PRs need two approvals to merge, but reviews from community members like yourself count!

@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 Jun 18, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 18, 2023
@NoahShomette
Copy link
Contributor

@NoahShomette thanks for the review :) In the future, feel free to actually use the Approve functionality in the Files Changed tab to make it easier to track. Bevy uses a system where PRs need two approvals to merge, but reviews from community members like yourself count!

Of course, thank you! Wasn't sure exactly how to do it but I'll get it done right next time!

Merged via the queue into bevyengine:main with commit 08962f1 Jun 19, 2023
25 checks passed
NoahShomette pushed a commit to NoahShomette/bevy that referenced this pull request Jun 19, 2023
- 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`
NoahShomette added a commit to NoahShomette/bevy that referenced this pull request Jun 19, 2023
mockersf pushed a commit to mockersf/bevy that referenced this pull request Jun 19, 2023
james7132 pushed a commit to james7132/bevy that referenced this pull request Jun 19, 2023
# 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`
NoahShomette pushed a commit to NoahShomette/bevy that referenced this pull request Jun 20, 2023
- 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`
@nicopap nicopap deleted the split_ecs_reflect branch August 30, 2023 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events A-Reflection Runtime information about types C-Code-Quality A section of code that is hard to understand or change 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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants