houseabsolute / DateTime-TimeZone

Time zone object base class and factory
https://metacpan.org/release/DateTime-TimeZone/
Other
9 stars 25 forks source link

Get --old timezones into a usable and testable state #1

Closed alfiedotwtf closed 9 years ago

alfiedotwtf commented 10 years ago

Hey,

Timezones like 'Etc/GMT+5' end up creating modules like 'DateTime/TimeZone/Etc/GMT+5.pm'. The problem here is that they are not valid Perl module names, so they can't be use'd. These changes fixes this, creates correct offsets, and allows "parse_olson --old" to pass all tests again.

autarch commented 10 years ago

Sorry for the ridiculously slow response. If you could redo this branch without checking in the generated files that'd be good. It's really hard to review as-is.

alfiedotwtf commented 10 years ago

Oops. Missed this one in my inbox. I'll try to do this sometime this week.

alfiedotwtf commented 10 years ago

Let me know if you want anything changed :)

autarch commented 10 years ago

I don't really like the way you have the zone class files names. Having both Etc::GMT_4 and Etc::GMT__4 seems really confusing. Maybe call them Etc::GMT_M4 and Etc::GMT_P4, although the "minus" vs "plus" thing is totally backwards with these zones. I'd welcome a better suggestion, but I think distinguishing the two based on the number of underscores is not a great solution.

Thanks,

-dave

alfiedotwtf commented 10 years ago

Code has been updated with your suggestions, but instead of M/P let's be more explicit i.e:

alfiedotwtf commented 10 years ago

Hey Dave,

Have you been able to have a look again?

Alfie

autarch commented 10 years ago

I was crazy busy with http;//www.tcvegfest.com/ until quite recently. I'll try to take a look some time this month.

alfiedotwtf commented 10 years ago

All good :) Thanks for the update.

autarch commented 9 years ago

I merged this manually. Note that I'm not going to enable --old by default so if you want these zones you'd have to rebuild the package from scratch.

alfiedotwtf commented 9 years ago

Hey Dave,

Awesome. Thanks for doing that.