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.

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

Panic Documentation #638

Merged
merged 2 commits into from
Dec 1, 2023
Merged

Panic Documentation #638

merged 2 commits into from
Dec 1, 2023

Conversation

gsquire
Copy link
Contributor

@gsquire gsquire commented Nov 29, 2023

This is an attempt to add some documentation for #623.

This patch adds some documentation for functions that may panic.
Copy link
Member

@jhpratt jhpratt left a comment

Choose a reason for hiding this comment

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

Definitely a start! I only have a couple minor changes; it otherwise LGTM. The one for Instant actually made me think that it can be worked around, but as it's not yet, the comment is correct.

Also, don't worry about the coverage CI check. It's broken at the moment due to a bug in the Rust compiler.

@@ -1224,6 +1236,9 @@ impl From<DateTime<offset_kind::Fixed>> for SystemTime {
feature = "wasm-bindgen"
))]
impl From<js_sys::Date> for DateTime<offset_kind::Fixed> {
/// # Panics
///
/// This may panic if the timestamp can not be represented by the return type.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// This may panic if the timestamp can not be represented by the return type.
/// This may panic if the timestamp can not be represented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated as part of a single commit.

@@ -1289,6 +1315,9 @@ impl Add<Duration> for StdDuration {
impl_add_assign!(Duration: Self, StdDuration);

impl AddAssign<Duration> for StdDuration {
/// # Panics
///
/// This may panic if the resulting addition cannot be represented by the return type.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// This may panic if the resulting addition cannot be represented by the return type.
/// This may panic if the resulting addition cannot be represented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated as part of a single commit.

@@ -1336,6 +1374,9 @@ impl Sub<Duration> for StdDuration {
impl_sub_assign!(Duration: Self, StdDuration);

impl SubAssign<Duration> for StdDuration {
/// # Panics
///
/// This may panic if the resulting subtraction can not be represented by the return type.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// This may panic if the resulting subtraction can not be represented by the return type.
/// This may panic if the resulting subtraction can not be represented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated as part of a single commit.

@gsquire
Copy link
Contributor Author

gsquire commented Nov 30, 2023

Thanks for the review. I thought the code coverage was a separate issue so I ignored it for now. Let me know if you would like me to squash my commits.

@jhpratt
Copy link
Member

jhpratt commented Dec 1, 2023

Thanks! No need to squash; I can do that on my end.

@jhpratt jhpratt merged commit 6d5fa1e into time-rs:main Dec 1, 2023
18 of 19 checks passed
@gsquire gsquire deleted the add-panic-docs branch December 4, 2023 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants