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

Refactor systems.rs separate files #509

Merged
merged 5 commits into from
May 17, 2024
Merged

Conversation

Vrixyz
Copy link
Contributor

@Vrixyz Vrixyz commented May 15, 2024

Same approach as #412, Thanks @Aceeri !

a few notes:

  • not sure what strategy to apply to use statements, when to glob or not.
  • I'm tempted to add update_colliding_entities to the collider mod.
  • to "minify" changes I kept the original systems.rs instead of creating a mod.rs (I personally prefer named modules). I'd understand we prefer consistency with other modules so I'll go the extra mile after confirmation.

@Vrixyz Vrixyz marked this pull request as ready for review May 15, 2024 15:19
@Vrixyz Vrixyz requested a review from sebcrozet May 15, 2024 15:19
Copy link
Member

@sebcrozet sebcrozet left a comment

Choose a reason for hiding this comment

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

Looking great thank you!

not sure what strategy to apply to use statements, when to glob or not.

The code base doesn’t always applies that strategy, but, my preference would be to glob-import the bevy prelude and import the rest explicitly.

I'm tempted to add update_colliding_entities to the collider mod.

Agreed. We could also move the writeback system to a writeback.rs file, and the removal handling to a remove.rs file.

to "minify" changes I kept the original systems.rs instead of creating a mod.rs (I personally prefer named modules). I'd understand we prefer consistency with other modules so I'll go the extra mile after confirmation.

Yeah, creating a mod.rs would be great. It might be a bit old-school, but I find its much cleaner to organize all files related to the same module in a single directory.

@Vrixyz Vrixyz requested a review from sebcrozet May 16, 2024 09:53
Copy link
Member

@sebcrozet sebcrozet left a comment

Choose a reason for hiding this comment

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

Fantastic, thanks!

@Vrixyz Vrixyz merged commit 3455d5f into dimforge:master May 17, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants