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

Expose MIN_TIME and MAX_TIME of NaiveTime #713

Closed
wants to merge 1 commit into from

Conversation

rikvdkleij
Copy link

@djc
Copy link
Contributor

djc commented Jun 20, 2022

Hmm, would it make more sense to expose these as NaiveTime::{MIN, MAX} associated types? I think that would be more idiomatic in modern Rust, at the cost of some consistency with current constants. We could introduce DateTime::{MIN_UTC, MAX_UTC} to fix the consistency, deprecating the other constants. @esheppa thoughts?

@esheppa
Copy link
Collaborator

esheppa commented Jun 20, 2022

@djc - I think making them associated is a great idea. Given the two options, either associated constants on the trait or associated constants in the inherent impl, I'd lean toward the latter, among other reasons, people are likely to have the type names already imported, but potentially not the traits, so its more ergonomic to get at the constants if they are associated constants on the inherent impl. (This is the same IDE friendliness reasoning that was persuasive in #711)

@djc
Copy link
Contributor

djc commented Jun 20, 2022

Okay, so let's do the associated consts (with the _UTC suffix in the case of types that take a TimeZone argument) and deprecate all the old MIN_ and MAX_ constants, with a note in the deprecation pointing to the new API.

@rikvdkleij does that seem like something you can tackle?

@rikvdkleij
Copy link
Author

@rikvdkleij does that seem like something you can tackle?

@djc No, sorry, it was just my intention to get access to the MIN_TIME constant and make it consistent with the other constants.

@djc
Copy link
Contributor

djc commented Jul 4, 2022

Fixed this with #726 instead.

@djc djc closed this Jul 4, 2022
@djc
Copy link
Contributor

djc commented Jul 4, 2022

I guess that PR doesn't yet actually expose those values, so feel free to submit a new PR for that.

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

3 participants