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

Fix naming on "tick" Column and ComponentSparseSet methods #9744

Merged
merged 1 commit into from
Sep 11, 2023

Conversation

nicopap
Copy link
Contributor

@nicopap nicopap commented Sep 10, 2023

Objective

  • The tick access methods mention "ticks" (as in: plural). Yet, most of them only access a single tick.

Solution

  • Rename those methods and fix docs to reflect the singular aspect of the return values

Migration Guide

The following method names were renamed, from foo_ticks_bar to foo_tick_bar (ticks is now singular, tick):

  • ComponentSparseSet::get_added_ticksget_added_tick
  • ComponentSparseSet::get_changed_ticksget_changed_tick
  • Column::get_added_ticksget_added_tick
  • Column::get_changed_ticksget_changed_tick
  • Column::get_added_ticks_uncheckedget_added_tick_unchecked
  • Column::get_changed_ticks_uncheckedget_changed_tick_unchecked

@nicopap nicopap added A-ECS Entities, components, systems, and events M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Sep 10, 2023
@nicopap nicopap changed the title Fix naming on "tick" Table and SparseSet methods Fix naming on "tick" Column and ComponentSparseSet methods Sep 10, 2023
@alice-i-cecile alice-i-cecile added the C-Code-Quality A section of code that is hard to understand or change label Sep 10, 2023
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.

Sure, I think that's more clear.

Copy link
Member

@JoJoJet JoJoJet left a comment

Choose a reason for hiding this comment

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

To ease migration, can you add deprecated methods with the old names that direct users to the new names?

@nicopap
Copy link
Contributor Author

nicopap commented Sep 11, 2023

@JoJoJet I think the compiler should be good enough at directing users. The name are similar enough that the error message from the error should include the updated name and be enough guidance. Since releases are expected to be breaking, I don't think a deprecation is needed in this case.

@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 Sep 11, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 11, 2023
Merged via the queue into bevyengine:main with commit 19c5357 Sep 11, 2023
26 checks passed
@nicopap nicopap deleted the added_tick_table_typo_fix branch September 20, 2023 13:16
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
…e#9744)

# Objective

- The tick access methods mention "ticks" (as in: plural). Yet, most of
them only access a single tick.

## Solution

- Rename those methods and fix docs to reflect the singular aspect of
the return values

---

## Migration Guide

The following method names were renamed, from `foo_ticks_bar` to
`foo_tick_bar` (`ticks` is now singular, `tick`):
- `ComponentSparseSet::get_added_ticks` → `get_added_tick`
- `ComponentSparseSet::get_changed_ticks` → `get_changed_tick`
- `Column::get_added_ticks` → `get_added_tick`
- `Column::get_changed_ticks` → `get_changed_tick`
- `Column::get_added_ticks_unchecked` → `get_added_tick_unchecked`
- `Column::get_changed_ticks_unchecked` → `get_changed_tick_unchecked`
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 C-Code-Quality A section of code that is hard to understand or change 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
Development

Successfully merging this pull request may close these issues.

4 participants