-
Notifications
You must be signed in to change notification settings - Fork 595
Add quarter (%q) date string specifier #1666
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,14 @@ pub trait Datelike: Sized { | |
| if year < 1 { (false, (1 - year) as u32) } else { (true, year as u32) } | ||
| } | ||
|
|
||
| /// Returns the quarter number starting from 1. | ||
| /// | ||
| /// The return value ranges from 1 to 4. | ||
| #[inline] | ||
| fn quarter(&self) -> u32 { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just as an FYI this resulted in our arrow-rs builds failing with the latest chrono patch release. We have codepaths generic on I don't really know if this counts as a breaking change or not... I feel it shouldn't, but it broke our build so maybe it is?? I don't know... Edit: The official guidance on semver highlights this as a potential issue but says it is up to maintainers to decide - https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-default-item
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-default-item. In general, with extension traits you're going to be somewhat more likely to be broken, especially if you pick very generic method names. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I am a little unsure how best to proceed here, in particular what changes arrow-rs should be making to ensure this doesn't occur again. Other than pinning chrono to a specific patch version, I can't see a tractable way to avoid a reoccurrence? Switching all callsites to use explicit disambiguation syntax when a chrono trait is in scope is not really a viable option given the extent to which the codebase makes use of traits and generics, and that isn't even considering our downstream consumers... Maybe it is a case of do nothing now and only do something if it happens again, but I also wonder if chrono might consider not adding new methods to traits in patch releases, especially given how broad its user base is?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wouldn't worry about it too much, I think this will be relatively rare.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is tenable. Unlike arrow which does regular incompatible releases, chrono is firmly entrenched as low-level library for which semver-incompatible bumps are pretty painful.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you want to influence the chrono roadmap feel free to contact me for a commercial arrangement. I'm not going to yank this release because your team is unable to manage There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @djc why not just yank the release and push this as 0.5.0 like semver conventions would suggest?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Your understanding of semver conventions as practiced in the crates.io ecosystem seems limited at best, and you seemingly have no idea of the impact of semver-incompatible releases for foundational libraries like chrono. Please take your uninformed "why don't you just" elsewhere. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Hi, it is allowed by the ASF to publish hot fixes without a three-day voting period after a public discussion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @djc If chrono is "firmly entrenched as a low-level library", maybe it's time for it to graduate to version 1.x? Regardless of whether publishing breaking changes to patch-level increments is considered OK by Cargo's semver specification, it displays a degree of immaturity for something as widely used as chrono to do that. |
||
| (self.month() - 1).div_euclid(3) + 1 | ||
| } | ||
|
|
||
| /// Returns the month number starting from 1. | ||
| /// | ||
| /// The return value ranges from 1 to 12. | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.