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.

Sign up for GitHub

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

Jump to bottom

Rename Command's "write" method to "apply" #8814

Merged
merged 2 commits into from
Jun 12, 2023
Merged

Rename Command's "write" method to "apply" #8814

merged 2 commits into from
Jun 12, 2023

Conversation

neithanmo
Copy link
Contributor

@neithanmo neithanmo commented Jun 11, 2023 •
edited
Loading

Objective

Solution

  • Rename "write" method to "apply" in Command trait definition.
  • Rename other implementations of command trait throughout bevy's code base.

Changelog

  • Changed: Command::write has been changed to Command::apply
  • Changed: EntityCommand::write has been changed to EntityCommand::apply

Migration Guide

  • Command::write implementations need to be changed to implement Command::apply instead. This is a mere name change, with no further actions needed.
  • EntityCommand::write implementations need to be changed to implement EntityCommand::apply instead. This is a mere name change, with no further actions needed.

@github-actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@neithanmo neithanmo force-pushed the rename_command_apply branch 2 times, most recently from f55a027 to e252ac9 Compare June 11, 2023 15:00
@neithanmo neithanmo force-pushed the rename_command_apply branch from e252ac9 to 646c07c Compare June 11, 2023 15:16
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jun 11, 2023
@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jun 11, 2023
@alice-i-cecile
Copy link
Member

Looks great, thank you! I won't block on it, but let me add a suggestion to improve the docs further...

@neithanmo @alice-i-cecile
Update crates/bevy_ecs/src/system/commands/mod.rs
e362801 
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@Themayu
Copy link
Contributor

Themayu commented Jun 11, 2023 •
edited
Loading

I would change the migration guide section in the PR body to be more concise and better fit a list format, as it is used to auto-generate the final migration guide in the changelog. A better wording would, in my opinion, be something along the following lines:

  • Command::write implementations need to be changed to implement Command::apply instead. This is a mere name change, with no further actions needed.

Also add this line to the migration guide:

  • EntityCommand::write implementations need to be changed to implement EntityCommand::apply instead. This is a mere name change, with no further actions needed.

And this line to the changelog:

  • Changed: EntityCommand::write has been changed to EntityCommand::apply

As EntityCommand was also changed in this PR.

@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 11, 2023
@neithanmo
Copy link
Contributor Author

neithanmo commented Jun 11, 2023 •
edited
Loading

I would change the migration guide section in the PR body to be more concise and better fit a list format, as it is used to auto-generate the final migration guide in the changelog. A better wording would, in my opinion, be something along the following lines:

  • Command::write implementations need to be changed to implement Command::apply instead. This is a mere name change, with no further actions needed.

Also add this line to the migration guide:

  • EntityCommand::write implementations need to be changed to implement EntityCommand::apply instead. This is a mere name change, with no further actions needed.

And this line to the changelog:

  • Changed: EntityCommand::write has been changed to EntityCommand::apply

As EntityCommand was also changed in this PR.

Done!
Thanks @Themayu for the suggestions

james7132
james7132 approved these changes Jun 12, 2023
View reviewed changes
crates/bevy_ecs/src/system/commands/mod.rs
///
/// This method is used to define what a command "does" when it is ultimately applied.
/// Because this method takes `self`, you can store data or settings on the type that implements this trait.
/// This data is set by the system or other source of the command, and then ultimately read in this method.
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the more detailed docs here!

@james7132 james7132 added this pull request to the merge queue Jun 12, 2023
@james7132 james7132 added this to the 0.11 milestone Jun 12, 2023
Merged via the queue into bevyengine:main with commit f135535 Jun 12, 2023
This was referenced Jun 28, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@mockersf mockersf mockersf approved these changes

@james7132 james7132 james7132 approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events 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 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
Milestone
0.11
Development

Successfully merging this pull request may close these issues.

Rename Command::write
5 participants
@neithanmo @alice-i-cecile @Themayu @james7132 @mockersf