[Merged by Bors] - Replace the `bool` argument of `Timer` with `TimerMode` #6247

lovelymono · 2022-10-13T11:06:16Z

As mentioned in #2926, it's better to have an explicit type that clearly communicates the intent of the timer mode rather than an opaque boolean, which can be only understood when knowing the signature or having to look up the documentation.

This also opens up a way to merge different timers, such as Stopwatch, and possibly future ones, such as DiscreteStopwatch and DiscreteTimer from #2683, into one struct.

Signed-off-by: Lena Milizé me@lvmn.org

Objective

Fixes #2926.

Solution

Introduce TimerMode which replaces the bool argument of Timer constructors. A Default value for TimerMode is Once.

Changelog

Added

TimerMode enum, along with variants TimerMode::Once and TimerMode::Repeating

Changed

Replace bool argument of Timer::new and Timer::from_seconds with TimerMode
Change repeating: bool field of Timer with mode: TimerMode

Migration Guide

Replace Timer::new(duration, false) with Timer::new(duration, TimerMode::Once).
Replace Timer::new(duration, true) with Timer::new(duration, TimerMode::Repeating).
Replace Timer::from_seconds(seconds, false) with Timer::from_seconds(seconds, TimerMode::Once).
Replace Timer::from_seconds(seconds, true) with Timer::from_seconds(seconds, TimerMode::Repeating).
Change timer.repeating() to timer.mode() == TimerMode::Repeating.

As mentioned in #2926, it's better to have an explicit type that clearly communicates the intent of the timer mode rather than an opaque boolean, which can be only understood when knowing the signature or reading the documentation. Signed-off-by: Lena Milizé <me@lvmn.org>

crates/bevy_time/src/timer.rs

tguichaoua

IMHO, I think all usage of bool for repeating should be completly replaced by TimerMode to avoid mixing and confusion.

crates/bevy_time/src/timer.rs

Replaces Timer::set_repeating and Timer::repeating with Timer::set_mode and Timer::mode, respectively. Adds PartialEq, Eq, Hash to TimerMode. Replaces impl Default with derive macro. Signed-off-by: Lena Milizé <me@lvmn.org>

Signed-off-by: Lena Milizé <me@lvmn.org>

tguichaoua · 2022-10-13T14:14:04Z

@lovelymono don't forgot to click on "Mark as resolved" on conversations. It's help other reviewers to know what is already done.

alice-i-cecile

Much clearer! This is a great first PR.

Signed-off-by: Lena Milizé <me@lvmn.org>

tguichaoua: > The problem with utility methods is that if you want to be consistant > you have to add one for each variant for each enum. That is complex to > maintain. > For fieldless enums using equality is more straightforward and seems > to be the most used way in the rust community. Signed-off-by: Lena Milizé <me@lvmn.org>

crates/bevy_time/src/timer.rs

We want to check that the timer is what we set it to, and before the condition was backwards. Signed-off-by: Lena Milizé <me@lvmn.org>

alice-i-cecile · 2022-10-17T13:34:46Z

bors r+

As mentioned in #2926, it's better to have an explicit type that clearly communicates the intent of the timer mode rather than an opaque boolean, which can be only understood when knowing the signature or having to look up the documentation. This also opens up a way to merge different timers, such as `Stopwatch`, and possibly future ones, such as `DiscreteStopwatch` and `DiscreteTimer` from #2683, into one struct. Signed-off-by: Lena Milizé <me@lvmn.org> # Objective Fixes #2926. ## Solution Introduce `TimerMode` which replaces the `bool` argument of `Timer` constructors. A `Default` value for `TimerMode` is `Once`. --- ## Changelog ### Added - `TimerMode` enum, along with variants `TimerMode::Once` and `TimerMode::Repeating` ### Changed - Replace `bool` argument of `Timer::new` and `Timer::from_seconds` with `TimerMode` - Change `repeating: bool` field of `Timer` with `mode: TimerMode` ## Migration Guide - Replace `Timer::new(duration, false)` with `Timer::new(duration, TimerMode::Once)`. - Replace `Timer::new(duration, true)` with `Timer::new(duration, TimerMode::Repeating)`. - Replace `Timer::from_seconds(seconds, false)` with `Timer::from_seconds(seconds, TimerMode::Once)`. - Replace `Timer::from_seconds(seconds, true)` with `Timer::from_seconds(seconds, TimerMode::Repeating)`. - Change `timer.repeating()` to `timer.mode() == TimerMode::Repeating`.

bors · 2022-10-17T14:01:19Z

Pull request successfully merged into main.

Build succeeded:

) As mentioned in bevyengine#2926, it's better to have an explicit type that clearly communicates the intent of the timer mode rather than an opaque boolean, which can be only understood when knowing the signature or having to look up the documentation. This also opens up a way to merge different timers, such as `Stopwatch`, and possibly future ones, such as `DiscreteStopwatch` and `DiscreteTimer` from bevyengine#2683, into one struct. Signed-off-by: Lena Milizé <me@lvmn.org> # Objective Fixes bevyengine#2926. ## Solution Introduce `TimerMode` which replaces the `bool` argument of `Timer` constructors. A `Default` value for `TimerMode` is `Once`. --- ## Changelog ### Added - `TimerMode` enum, along with variants `TimerMode::Once` and `TimerMode::Repeating` ### Changed - Replace `bool` argument of `Timer::new` and `Timer::from_seconds` with `TimerMode` - Change `repeating: bool` field of `Timer` with `mode: TimerMode` ## Migration Guide - Replace `Timer::new(duration, false)` with `Timer::new(duration, TimerMode::Once)`. - Replace `Timer::new(duration, true)` with `Timer::new(duration, TimerMode::Repeating)`. - Replace `Timer::from_seconds(seconds, false)` with `Timer::from_seconds(seconds, TimerMode::Once)`. - Replace `Timer::from_seconds(seconds, true)` with `Timer::from_seconds(seconds, TimerMode::Repeating)`. - Change `timer.repeating()` to `timer.mode() == TimerMode::Repeating`.

Updated Getting Started docs to match the change at bevyengine/bevy#6247 and replace from_seconds(time, bool) with from_seconds(time, TimerMode)

Updated Getting Started: Resources docs to match the change at bevyengine/bevy#6247 and replace from_seconds(time, bool) with from_seconds(time, TimerMode). Co-authored-by: thejohnbackes <jwbackes@gmail.com>

) As mentioned in bevyengine#2926, it's better to have an explicit type that clearly communicates the intent of the timer mode rather than an opaque boolean, which can be only understood when knowing the signature or having to look up the documentation. This also opens up a way to merge different timers, such as `Stopwatch`, and possibly future ones, such as `DiscreteStopwatch` and `DiscreteTimer` from bevyengine#2683, into one struct. Signed-off-by: Lena Milizé <me@lvmn.org> # Objective Fixes bevyengine#2926. ## Solution Introduce `TimerMode` which replaces the `bool` argument of `Timer` constructors. A `Default` value for `TimerMode` is `Once`. --- ## Changelog ### Added - `TimerMode` enum, along with variants `TimerMode::Once` and `TimerMode::Repeating` ### Changed - Replace `bool` argument of `Timer::new` and `Timer::from_seconds` with `TimerMode` - Change `repeating: bool` field of `Timer` with `mode: TimerMode` ## Migration Guide - Replace `Timer::new(duration, false)` with `Timer::new(duration, TimerMode::Once)`. - Replace `Timer::new(duration, true)` with `Timer::new(duration, TimerMode::Repeating)`. - Replace `Timer::from_seconds(seconds, false)` with `Timer::from_seconds(seconds, TimerMode::Once)`. - Replace `Timer::from_seconds(seconds, true)` with `Timer::from_seconds(seconds, TimerMode::Repeating)`. - Change `timer.repeating()` to `timer.mode() == TimerMode::Repeating`.

tguichaoua suggested changes Oct 13, 2022

View reviewed changes

crates/bevy_time/src/timer.rs Outdated Show resolved Hide resolved

crates/bevy_time/src/timer.rs Outdated Show resolved Hide resolved

crates/bevy_time/src/timer.rs Outdated Show resolved Hide resolved

tguichaoua suggested changes Oct 13, 2022

View reviewed changes

crates/bevy_time/src/timer.rs Outdated Show resolved Hide resolved

crates/bevy_time/src/timer.rs Outdated Show resolved Hide resolved

lovelymono added 2 commits October 13, 2022 15:26

replace set_repeating with set_mode

4881f87

Replaces Timer::set_repeating and Timer::repeating with Timer::set_mode and Timer::mode, respectively. Adds PartialEq, Eq, Hash to TimerMode. Replaces impl Default with derive macro. Signed-off-by: Lena Milizé <me@lvmn.org>

fix doc tests and tests after 4881f87

3e6cbd1

Signed-off-by: Lena Milizé <me@lvmn.org>

lovelymono requested a review from tguichaoua October 13, 2022 13:51

alice-i-cecile added C-Usability A simple quality-of-life change that makes Bevy easier to use A-Time Involves time keeping and reporting C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide labels Oct 13, 2022

alice-i-cecile approved these changes Oct 13, 2022

View reviewed changes

lovelymono added 2 commits October 13, 2022 16:45

replace assert!(timer.mode() == Repeating) with assert_eq!

d5f752e

Signed-off-by: Lena Milizé <me@lvmn.org>

tguichaoua approved these changes Oct 13, 2022

View reviewed changes

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 Oct 13, 2022

mockersf reviewed Oct 13, 2022

View reviewed changes

crates/bevy_time/src/timer.rs Outdated Show resolved Hide resolved

replace assert_ne with assert_eq

88ddcc5

We want to check that the timer is what we set it to, and before the condition was backwards. Signed-off-by: Lena Milizé <me@lvmn.org>

SleepySwords mentioned this pull request Oct 14, 2022

[Merged by Bors] - Add default implementation of Serialize and Deserialize to Timer and Stopwatch #6248

Closed

lovelymono requested a review from mockersf October 14, 2022 11:08

bors bot changed the title ~~Replace the bool argument of Timer with TimerMode~~ [Merged by Bors] - Replace the bool argument of Timer with TimerMode Oct 17, 2022

bors bot closed this Oct 17, 2022

lovelymono deleted the timer-mode branch October 17, 2022 14:06

mockersf added the hacktoberfest-accepted A PR that was accepted for Hacktoberfest, an annual open source event label Oct 17, 2022

jackbackes added a commit to jackbackes/bevy-website that referenced this pull request Nov 12, 2022

Update _index.md

bc0fe6d

Updated Getting Started docs to match the change at bevyengine/bevy#6247 and replace from_seconds(time, bool) with from_seconds(time, TimerMode)

jackbackes mentioned this pull request Nov 12, 2022

[Merged by Bors] - Update "Getting Started: Resources" to include TimerMode bevyengine/bevy-website#480

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Replace the `bool` argument of `Timer` with `TimerMode` #6247

[Merged by Bors] - Replace the `bool` argument of `Timer` with `TimerMode` #6247

lovelymono commented Oct 13, 2022 •

edited

tguichaoua left a comment

tguichaoua commented Oct 13, 2022

alice-i-cecile left a comment

alice-i-cecile commented Oct 17, 2022

bors bot commented Oct 17, 2022

[Merged by Bors] - Replace the bool argument of Timer with TimerMode #6247

[Merged by Bors] - Replace the bool argument of Timer with TimerMode #6247

Conversation

lovelymono commented Oct 13, 2022 • edited

Objective

Solution

Changelog

Added

Changed

Migration Guide

tguichaoua left a comment

Choose a reason for hiding this comment

tguichaoua commented Oct 13, 2022

alice-i-cecile left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Oct 17, 2022

bors bot commented Oct 17, 2022

[Merged by Bors] - Replace the `bool` argument of `Timer` with `TimerMode` #6247

[Merged by Bors] - Replace the `bool` argument of `Timer` with `TimerMode` #6247

lovelymono commented Oct 13, 2022 •

edited