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

ZonedDateTime::toDateTime and getDateTime conflict #47

Closed
solodkiy opened this issue Apr 10, 2022 · 3 comments
Closed

ZonedDateTime::toDateTime and getDateTime conflict #47

solodkiy opened this issue Apr 10, 2022 · 3 comments

Comments

@solodkiy
Copy link
Contributor

solodkiy commented Apr 10, 2022

In ZonedDateTime class we have two methods which looks almost the same, but different in return types:

  • public function toDateTime() : \DateTime
  • public function getDateTime() : LocalDateTime

To avoid misunderstanding and confuse I propose rename toDateTime to toNativeDateTime.
This approach could be used in other conversions in and from native types. For example:

  • Period::fromDateInterval -> Period::fromNativeDateInterval
  • Period::toDateInterval -> Period::toNativeDateInterval
  • LocalDateRange::toDatePeriod -> LocalDateRange::toNativeDatePeriod
  • TimeZone::fromDateTimeZone -> TimeZone::fromNativeDateTimeZone
    etc..

Another variant is use Php prefix (toDateTime -> toPhpDateTime).

@tigitz
Copy link
Contributor

tigitz commented Apr 24, 2022

IMO, the Native prefix makes sense, I'm 👍 on this it helps readability and reduce ambiguity. I've wondered myself while trying to autocomplete method calls if the suggested DatePeriod for example was a library class or the native one.

However I would keep the getDateTime as it is since it means it is getting the DateTime part of the ZonedDateTime. Using the to prefix would suggest it's transforming the object into another one which is not the case here.

@BenMorel wdyt ?

@solodkiy
Copy link
Contributor Author

solodkiy commented Jun 5, 2022

Some of get*()/to*() discussion was there: #48

@BenMorel
Copy link
Member

BenMorel commented Jun 5, 2022

LGTM as well with the Native prefix. @solodkiy You're welcome to open a PR to create the new (to|from)Native*() methods, and deprecate existing ones. We'll remove them in 0.5.0.

The getDateTime() method is in line with the discussion we had in #48, so it looks good as it is!

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

No branches or pull requests

3 participants