Skip to content

Prefer core::time::Duration over std::time::Duration #18313

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

Closed
wants to merge 1 commit into from

Conversation

mnmaita
Copy link
Member

@mnmaita mnmaita commented Mar 14, 2025

Objective

  • While working on a bevy 0.15 project and then checking the current state of the main bevy branch to see why my autocomplete wasn't picking up the bevy::utils::Duration export I noticed that I couldn't find it anymore. As far as I can see Remove everything except Instant from bevy_utils::time #17158 removed this re-export, yet some parts of the code still use std::time::Duration. To standardize this I'm replacing all instances of std::time::Duration in favor of core::time::Duration. These are mostly example code and comments for what is worth.

Solution

  • Replaced all instances of std::time::Duration in favor of core::time::Duration.

Testing

  • I haven't tested this changeset thoroughly as I think its impact should be negligible. Might be good to perform an example run on CI though.

@mnmaita mnmaita added C-Examples An addition or correction to our examples C-Code-Quality A section of code that is hard to understand or change A-Cross-Cutting Impacts the entire engine D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Mar 14, 2025
@mnmaita mnmaita force-pushed the mnmaita/core-duration branch from 845258f to a0e179e Compare March 14, 2025 17:39
@alice-i-cecile
Copy link
Member

Generally we don't worry about no_std in examples / test / benchmarks, which is why these are still around.

@alice-i-cecile alice-i-cecile added the O-Embedded Weird hardware and no_std platforms label Mar 14, 2025
Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

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

Personally, I think there's no harm here and it is more consistent with how the Bevy crates are written now. However, I did propose this before and @mockersf raised a good point that this is not how the majority of Bevy users will write code, so it negatively affects the utility of the examples.

To be clear, I support this change, but this is at the very least controversial.

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Mar 15, 2025
@mockersf
Copy link
Member

Generally we don't worry about no_std in examples / test / benchmarks, which is why these are still around.

mostly seconding this, but I don't think it matters for benchmarks.

The change in example window/change_window_mode.rs is a pathological case of why this isn't really good:

            std::thread::sleep(core::time::Duration::from_secs(4));

We are using std threads there, why introduce confusion?

@mnmaita mnmaita closed this Mar 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Cross-Cutting Impacts the entire engine C-Code-Quality A section of code that is hard to understand or change C-Examples An addition or correction to our examples D-Straightforward Simple bug fixes and API improvements, docs, test and examples O-Embedded Weird hardware and no_std platforms S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants