-
-
Notifications
You must be signed in to change notification settings - Fork 10
feat: expose Period::new by introducing a PeriodUnit enum
#47
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
Conversation
|
It's unclear to me what the benefit of this change might be. This change definitely already breaks the public API, so it doesn't make much sense to retain some parts of it. Also note that this crate is currently being maintained on a volunteer basis. If you're going to start depending on it for business-critical purposes, I suggest we should have a conversation about commercial support. |
|
Did I "break" the public API? My intent was to only extend it. Perhaps our definitions differ 😊 Good point. I'll send you a email later today after having a conversation with my bosses. Where would you prefer us to send it to? dirkjan@ochtman.nl? I have some fairly uncontroversial PRs lined up that I wish to send in during the meantime. Hope that that's ok by you. |
Ah, sorry -- I was mistaken. Would still be good to discuss why this is an improvement for you.
Yup, that should work!
I'll take a quick look and will decide whether I want to review as a volunteer per PR. 👍 |
8ead509 to
962e085
Compare
|
FWIW, I don't think "deprecate" is a great description for just killing API. :) |
|
Heheh, yeah. Should probably just have written "remove". |
The
Perdiod::yearsandPeriod::monthsmay feel redundant now, but keeping them to not break any public API.We're building an interface at work. Doing this change upstream removes the need for us (and probably others) to create a data type that solely acts as a bridge.