search

brick / date-time

Date and time library for PHP
MIT License
337 stars 30 forks source link

Make TimeZoneRegion::utc() return a TimeZoneRegion not an TimeZoneOffset #49

Open tigitz opened 2 years ago

tigitz commented 2 years ago

I think it's reasonable from a DX perspective to expect TimeZoneRegion::utc() to return a TimeZoneRegion instead of the offset.

tigitz commented 2 years ago

@BenMorel If you could take a quick look about this one, it's pretty straight forward. It's slightly related to https://github.com/brick/date-time/issues/55

jiripudil commented 2 years ago

Or perhaps there shouldn't be TimeZone::utc() at all, only leaving TimeZoneOffset::utc()? That's what Java does and it makes it super clear that it's an offset-based TZ.

I appreciate the shorthand form of TimeZone::utc() and I must admit we're using it a lot in our code, so both this PR and my proposal would be a breaking change, but if it would help clear some confusion, we could live with it; it's an easy change, after all.

BenMorel commented 2 years ago

Good point about TimeZoneRegion::utc() returning an offset, we definitely need to fix this in one way or another.

I did like the fact than you could just get UTC from the root TimeZone class, but it definitely makes it weird when called on TimeZoneRegion.

I don't think keeping an abstract static TimeZone::utc() makes sense though, it can probably be removed (in 0.5.0).

That leaves us with two methods to represent a UTC time-zone: TimeZoneOffset::utc() and TimeZoneRegion::utc(). Isn't this confusing? What should we typically use here?

Indeed, Java has a UTC constant on ZoneOffset, but not ZoneId. Maybe doing the same would introduce less confusion? I can imagine future issues popping from users asking what's the difference between TimeZoneOffset::utc() and TimeZoneRegion::utc().

solodkiy commented 2 years ago

Definitely don't want to see two different utc factory. Move utc() to TimeZoneOffset looks reasonable. Do we have any real use case for TimeZoneRegion::utc()?

BenMorel commented 2 years ago

@solodkiy I don't think so. You can still construct it manually with TimeZoneRegion::of('etc/UTC') anyway if required.

BenMorel commented 1 year ago

Moving forward, I don't think it hurts much to keep TimeZone::utc() for now, that delegates to TimeZoneOffset::utc() as it currently does.

@tigitz Can you please just add the new method in TimeZoneRegion, but not replace calls to TimeZone::utc() with TimeZoneOffset::utc()?