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

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

Closed
wants to merge 5 commits into from
Closed

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

wants to merge 5 commits into from

Conversation

alfiedotwtf
Copy link
Contributor

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
Copy link
Member

autarch commented Jun 26, 2014

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
Copy link
Contributor Author

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

@alfiedotwtf
Copy link
Contributor Author

Let me know if you want anything changed :)

@autarch
Copy link
Member

autarch commented Aug 30, 2014

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
Copy link
Contributor Author

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

  • Etc::GMT_Minus4
  • Etc::GMT_Plus4

@alfiedotwtf
Copy link
Contributor Author

Hey Dave,

Have you been able to have a look again?

Alfie

@autarch
Copy link
Member

autarch commented Oct 7, 2014

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

@alfiedotwtf
Copy link
Contributor Author

All good :) Thanks for the update.

@autarch
Copy link
Member

autarch commented Oct 26, 2014

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.

@autarch autarch closed this Oct 26, 2014
@alfiedotwtf
Copy link
Contributor Author

Hey Dave,

Awesome. Thanks for doing that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants