-
Notifications
You must be signed in to change notification settings - Fork 13k
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
doc: some improvements to Instant and SystemTime #30061
Conversation
r? @brson (rust_highfive has picked a reviewer for you, use r? to override) |
@bors r+ rollup |
📌 Commit 9bff112 has been approved by |
@@ -140,7 +140,7 @@ impl SystemTime { | |||
/// guaranteed to always be before later measurements (due to anomalies such | |||
/// as the system clock being adjusted either forwards or backwards). | |||
/// | |||
/// If successful, `Ok(duration)` is returned where the duration represents | |||
/// If successful, `Ok(Duration)` is returned where the duration represents |
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.
Why Duration
was capitalised? It is a variable, not a type. (type is Result<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.
I would argue that Ok(Duration) is indeed a type -- there's even proposals that would make this literally true.
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.
OTOH, the docs here aren't talking about the type
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.
There is no variable named duration
anywhere here, so I felt using a type name would be more suitable. It would also be implied that we are referring to the type of the returned value.
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 seems to be very opionated topic without a clear preference, so I retract my objection for the change, I guess.
@bors r- |
This looks good to go for me. Sorry for the false alarm again. |
@bors r=brson rollup |
📌 Commit cc815d2 has been approved by |
💔 Test failed - auto-mac-64-nopt-t |
@bors: retry |
⚡ Previous build results for auto-linux-cross-opt, auto-linux-musl-64-opt, auto-win-msvc-32-opt are reusable. Rebuilding only auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-64-opt... |
No description provided.