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

aligning public apis of Time,Timer and Stopwatch #15962

Merged
merged 1 commit into from
Oct 16, 2024
Merged

aligning public apis of Time,Timer and Stopwatch #15962

merged 1 commit into from
Oct 16, 2024

Conversation

andristarr
Copy link
Contributor

@andristarr andristarr commented Oct 16, 2024 •
edited by alice-i-cecile
Loading

Fixes #15834

Migration Guide

The APIs of Time, Timer and Stopwatch have been cleaned up for consistency with each other and the standard library's Duration type. The following methods have been renamed:

  • Stowatch::paused -> Stopwatch::is_paused
  • Time::elapsed_seconds -> Time::elasped_secs (including _f64 and _wrapped variants)

@alice-i-cecile alice-i-cecile added 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 A-Time Involves time keeping and reporting D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 16, 2024
@github-actions GitHub Actions
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@alice-i-cecile
Copy link
Member

Oh sweet, I got the nag bot working again :D

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 16, 2024
@alice-i-cecile
Copy link
Member

I'll write the migration guide now.

alice-i-cecile
alice-i-cecile requested changes Oct 16, 2024
View reviewed changes
crates/bevy_time/src/time.rs Outdated
@@ -281,14 +281,14 @@ impl<T: Default> Time<T> {
/// seconds.
#[inline]
pub fn delta_seconds(&self) -> f32 {
Copy link
Member

Choose a reason for hiding this comment

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

We should rename these too actually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This probably wont be too fun to review :D

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Oct 16, 2024
@andristarr andristarr force-pushed the 15834.time-timer-stopwatch-api-align branch from 3663518 to 0809f10 Compare October 16, 2024 20:03
@andristarr andristarr force-pushed the 15834.time-timer-stopwatch-api-align branch from 0809f10 to 9f1d961 Compare October 16, 2024 20:16
ickshonpe
ickshonpe approved these changes Oct 16, 2024
View reviewed changes
Copy link
Contributor

@ickshonpe ickshonpe left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The from_seconds methods still need to be changed, or are you leaving the from_ methods with the full names?

Looks good otherwise. Ran a few of the examples that use timers and nothings broken. I like seconds more than secs, but don't care that much and the uniform api is worth it.

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Oct 16, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 16, 2024
Merged via the queue into bevyengine:main with commit 7482a0d Oct 16, 2024
29 checks passed
bas-ie added a commit to bas-ie/bevy_ecs_tilemap that referenced this pull request Oct 25, 2024
@bas-ie
elapsed_seconds -> elapsed_secs
38eccf9 
See: bevyengine/bevy#15962
@bas-ie bas-ie mentioned this pull request Oct 25, 2024
Closed
@mgi388 mgi388 mentioned this pull request Oct 25, 2024
Closed
rparrett pushed a commit to rparrett/bevy_ecs_tilemap that referenced this pull request Nov 20, 2024
@bas-ie @rparrett
elapsed_seconds -> elapsed_secs
6d1480d 
See: bevyengine/bevy#15962
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tim-blackbird tim-blackbird tim-blackbird approved these changes

@ickshonpe ickshonpe ickshonpe approved these changes

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

Assignees
No one assigned
Labels
A-Time Involves time keeping and reporting C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples 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
No milestone
Development

Successfully merging this pull request may close these issues.

Make methods on Time, Timer, and Stopwatch more consistent
4 participants
@andristarr @alice-i-cecile @ickshonpe @tim-blackbird