brick / date-time

Date and time library for PHP
MIT License
323 stars 29 forks source link

ZonedDateTime::toDateTime and getDateTime conflict #47

Closed solodkiy closed 2 years ago

solodkiy commented 2 years ago

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

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:

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

tigitz commented 2 years ago

IMO, the Native prefix makes sense, I'm :+1: 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 commented 2 years ago

Some of get*()/to*() discussion was there: https://github.com/brick/date-time/pull/48

BenMorel commented 2 years ago

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!