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

YearMonth: deprecate int argument for month, add support for Month enum. #96

Merged
merged 1 commit into from
Mar 23, 2024

Conversation

gnutix
Copy link
Contributor

@gnutix gnutix commented Mar 22, 2024

No description provided.

@gnutix gnutix force-pushed the yearmonth-should-support-month-enum branch 2 times, most recently from e9928ba to 13e0db8 Compare March 22, 2024 09:46
@gnutix gnutix force-pushed the yearmonth-should-support-month-enum branch from 13e0db8 to 3534c4e Compare March 22, 2024 09:50
@BenMorel
Copy link
Member

Side note: I was thinking of removing getMonth() in the next release (0.7), to re-introduce it later in 0.8 as returning a Month enum.

The other option would be to change the signature right away in 0.7, but I feel like skipping a version makes it more likely for people upgrading one version at a time to notice the change. Do you have an opinion on this?

@gnutix
Copy link
Contributor Author

gnutix commented Mar 23, 2024

Side note: I was thinking of removing getMonth() in the next release (0.7), to re-introduce it later in 0.8 as returning a Month enum.

The other option would be to change the signature right away in 0.7, but I feel like skipping a version makes it more likely for people upgrading one version at a time to notice the change. Do you have an opinion on this?

I have no experience releasing open source libraries. But if it's a BC Break, shouldn't it be done in a new major version, like 1.0 ? I was wondering why you don't have such a version yet, don't you think this library is stable / production-ready ? (I do)

Anyway, if you still want to release 0.x version and 0.6 had a lot of BC Breaks, I (as a user) wouldn't be surprised that 0.7 has some as well. So IMHO, no need to do it in two versions, it would just be annoying having to change our code multiple times.

@gnutix gnutix force-pushed the yearmonth-should-support-month-enum branch from 3534c4e to d12be76 Compare March 23, 2024 08:49
@gnutix gnutix requested a review from BenMorel March 23, 2024 09:17
@BenMorel
Copy link
Member

But if it's a BC Break, shouldn't it be done in a new major version, like 1.0 ? I was wondering why you don't have such a version yet, don't you think this library is stable / production-ready ? (I do)

semver allows breaking changes in version 0.x. I keep non-breaking changes in patch releases, and breaking changes in minor releases.

The library is definitely production quality, but I feel like the API is not stable yet. Once we reach the 1.0.0 release, we will have to release a major version for even the smallest of breaking changes. If I had released 1.0.0 from the beginning, we would be at version 6.0.0 already, with sometimes very few minor/patch releases in between major versions.

Tagging 1.0.0 one day, will mean that I consider the library mostly finished and the API stable.

Sure, didn't thought of the == here. There should be a unit test covering that!

Adding a unit test would mean that we support this use case, and I'm personally against using objects this way. Loose comparison is an unfortunate "feature" of PHP, which exposes object internals, and supporting it prevents using features like internal cache / lazy loading. We're a bit in a grey zone here, where I want to avoid the BC break if possible, but still not officially support this use case.

@BenMorel BenMorel added this to the 0.7 milestone Mar 23, 2024
@gnutix gnutix force-pushed the yearmonth-should-support-month-enum branch from d12be76 to 1f5e9c8 Compare March 23, 2024 13:03
@gnutix gnutix requested a review from BenMorel March 23, 2024 13:04
@BenMorel BenMorel merged commit 4219c38 into brick:master Mar 23, 2024
7 checks passed
@BenMorel
Copy link
Member

Thank you, @gnutix!

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