-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Add more Duration methods for consistency. #46508
Conversation
r? @KodrAus (rust_highfive has picked a reviewer for you, use r? to override) |
71e445d
to
a8bfd81
Compare
I'm on board with these methods! @rfcbot fcp merge |
Team member @sfackler has proposed to merge this. The next step is review by the rest of the tagged teams: No concerns currently listed. Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
Tidy's failing |
/// ``` | ||
#[unstable(feature = "duration_extras", issue = "46507")] | ||
#[inline] | ||
pub fn from_nanos(nanos: u64) -> Duration { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like it should be from_nanosecs
or something like that by the convention in other methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing constructors are new
, from_secs
, from_millis
, and from_micros
, so it seems like from_nanos
would match conventions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we've been somewhat inconsistent with abbreviating seconds already, so this seems fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally like the use of the pluralised prefices, considering how simply using ns
, us
, and ms
would be very easy to make typos. I don't personally like using sec
as an abbreviation for second
considering how sec
is secant and s
is second, but that's just because I'm a maths person and I appreciate clarity in equations. We shouldn't really change the naming scheme now that this has been stabilised for a long time.
I personally would rather that |
@retep998 A |
@retep998 |
003347c
to
09a1d5b
Compare
also @sfackler the build is passing now |
@clarcharr You can ask that question to whoever designed |
@retep998 I don't see why that's a problem. If you can't initialize a Duration via a number of nanoseconds, you can do it via |
Ah right, |
09a1d5b
to
2be7a31
Compare
Ping @aturon for ticky boxes! |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
@bors: r=sfackler |
📌 Commit 2be7a31 has been approved by |
Add more Duration methods for consistency. Follow-up to #46507.
☀️ Test successful - status-appveyor, status-travis |
@clarcharr Did you mean microseconds, because |
Follow-up to #46507.