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

Add FromStr for FixedOffset #1092

Closed
wants to merge 1 commit into from
Closed

Add FromStr for FixedOffset #1092

wants to merge 1 commit into from

Conversation

yshui
Copy link
Contributor

@yshui yshui commented May 28, 2023

Closes #314

Copy link
Collaborator

@pitdicker pitdicker left a comment

Choose a reason for hiding this comment

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

Seems like a sensible addition to me.

@@ -991,6 +992,15 @@ impl FromStr for Weekday {
}
}

/// Parsing a `str` into a `FixedOffset` uses the format [`%z`](./format/strftime/index.html).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Parsing a `str` into a `FixedOffset` uses the format [`%z`](./format/strftime/index.html).
/// Parsing a `str` into a `FixedOffset` uses the format [`%z`](crate::format::strftime).

Should take care of the CI warning.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this impl below the FixedOffset definition.

Copy link
Contributor

Choose a reason for hiding this comment

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

This addition should be move to src/offset/fixed.rs.

@@ -221,7 +221,7 @@ pub(super) fn trim1(s: &str) -> &str {

/// Consumes one colon char `:` if it is at the front of `s`.
/// Always returns `Ok(s)`.
pub(super) fn consume_colon_maybe(mut s: &str) -> ParseResult<&str> {
pub(crate) fn consume_colon_maybe(mut s: &str) -> ParseResult<&str> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

These two methods don't need to be public for the crate.

But maybe you should keep them like this, and put the FromStr implementation in offset/fixed.rs. It seems to be an exception to put them in this file.

Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

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

Makes sense, thanks!

@@ -991,6 +992,15 @@ impl FromStr for Weekday {
}
}

/// Parsing a `str` into a `FixedOffset` uses the format [`%z`](./format/strftime/index.html).
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this impl below the FixedOffset definition.

@@ -991,6 +992,15 @@ impl FromStr for Weekday {
}
}

/// Parsing a `str` into a `FixedOffset` uses the format [`%z`](./format/strftime/index.html).
Copy link
Contributor

Choose a reason for hiding this comment

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

This addition should be move to src/offset/fixed.rs.

Self::east_opt(offset).ok_or(OUT_OF_RANGE)
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This addition should be move to src/offset/fixed.rs.

@pitdicker
Copy link
Collaborator

Merged in #1157. Thank you for working on this.

@pitdicker pitdicker closed this Jun 29, 2023
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.

None yet

4 participants