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

Fix Month enum argument deprecations within the library itself. #102

Closed
wants to merge 1 commit into from

Conversation

gnutix
Copy link
Contributor

@gnutix gnutix commented Mar 27, 2024

Hello @BenMorel ,

I tried upgrading to 0.6.1 in our project, but it broke our Behat test suite because the library in itself is now triggering tons of deprecations via trigger_error(). Anyway, I tried to "finish the job" of using Month wherever there is a month concerned within the library itself, while leaving the usages of the |int alternative in the tests to make sure it still works.

I also had to use the Field\MonthOfYear::check($month); line before creating the Enum to keep the currently throwned DateTimeException, otherwise it would throw a PHP ValueError when given invalid values to BackendEnum::from().

Let me know what you think. I had to rush it quite a bit, so there's probably room for improvements. I'm not a fan of how the code looks right now, because of this compatibility layer, and even more because we don't store the Month instance internally, forcing to recreate it in a lot of places. But I'm also glad we don't change that, as already discussed. 😅

@gnutix gnutix force-pushed the fix-month-enum-deprecations branch from ad1c9a4 to 2433f3c Compare March 28, 2024 07:55
@BenMorel
Copy link
Member

@gnutix, I'm actually thinking of un-deprecating passing an int to these functions. After double-checking the original Java implementation, I noticed that they use method overloading to accept both int and Month whenever a month is accepted. A couple examples:

  • LocalDate:
    • static LocalDate of(int year, int month, int dayOfMonth)
    • static LocalDate of(int year, Month month, int dayOfMonth)
  • YearMonth:
    • static YearMonth of(int year, int month)
    • static YearMonth of(int year, Month month)

I feel like this gives greater flexibility to the library, WDYT? (You're still welcome to pass Month enums internally as per your PR though, if it makes the code more readable.)

Side note, I noticed that they strangely only accept int in withMonth() methods, I'm not sure why. But that shouldn't prevent us from accepting both.

Finally, could you please do one update per commit? Like:

  • Accept Month in LocalDate::of() & LocalDate::withMonth()
  • Accept Month in MonthDay::of() & MonthDay::withMonth()
  • ...
  • Convert internal calls to Month enum

This will make reviewing the code much easier. Thanks!

@gnutix
Copy link
Contributor Author

gnutix commented Mar 30, 2024

Un-deprecating works for me. That will indeed give more flexibility to users, and it's not complex to maintain both input types.

I've split the changes into two PRs : removing deprecations and adding Month enum support :

PS: sorry for the spam, it took me a few iterations to get it right

@gnutix gnutix closed this Mar 30, 2024
@gnutix gnutix deleted the fix-month-enum-deprecations branch March 30, 2024 09:26
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