Skip to content

feat: expose Period::new by introducing a PeriodUnit enum#47

Merged
djc merged 3 commits into
djc:mainfrom
gibbz00:perdiod_new
Oct 21, 2024
Merged

feat: expose Period::new by introducing a PeriodUnit enum#47
djc merged 3 commits into
djc:mainfrom
gibbz00:perdiod_new

Conversation

@gibbz00

@gibbz00 gibbz00 commented Oct 18, 2024

Copy link
Copy Markdown
Contributor

The Perdiod::years and Period::months may 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.

@djc

djc commented Oct 18, 2024

Copy link
Copy Markdown
Owner

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.

@gibbz00

gibbz00 commented Oct 18, 2024

Copy link
Copy Markdown
Contributor Author

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.

@djc

djc commented Oct 18, 2024

Copy link
Copy Markdown
Owner

Did I "break" the public API? My intent was to only extend it. Perhaps our definitions differ 😊

Ah, sorry -- I was mistaken. Would still be good to discuss why this is an improvement for you.

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?

Yup, that should work!

I have some fairly uncontroversial PRs lined up that I wish to send in during the meantime. Hope that that's ok by you.

I'll take a quick look and will decide whether I want to review as a volunteer per PR. 👍

Comment thread src/domain/mod.rs Outdated
Comment thread src/domain/mod.rs
Comment thread src/domain/mod.rs Outdated
Comment thread src/domain/mod.rs Outdated
@djc djc merged commit ef1746c into djc:main Oct 21, 2024
@djc

djc commented Oct 21, 2024

Copy link
Copy Markdown
Owner

FWIW, I don't think "deprecate" is a great description for just killing API. :)

@gibbz00 gibbz00 deleted the perdiod_new branch October 21, 2024 16:02
@gibbz00

gibbz00 commented Oct 21, 2024

Copy link
Copy Markdown
Contributor Author

Heheh, yeah. Should probably just have written "remove".

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