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

Update time zone abbreviations #104

Closed
wants to merge 3 commits into from

Conversation

matthewbadeau
Copy link

@matthewbadeau matthewbadeau commented Jun 30, 2020

Uses list of time zone abbreviations. https://en.wikipedia.org/wiki/List_of_time_zone_abbreviations

I expanded the regular expression to use all time zone abbreviations.

As an example AEST time zones are being clipped to just EST. This change fixes that problem. It also fixes unreported problems with other time zones. According to Wikipedia the longest time zone is five characters.

@chase-manning
Copy link
Collaborator

@matthewbadeau Hi Matthew, thanks so much for the PR!!
Sorry this was not merged sooner, the repo was not being actively maintained at the time.
I would love to merge this PR now, but it has a merge conflict.
Would you be able to please resolve this merge conflict so I can merge it?

@matthewbadeau
Copy link
Author

@chase-manning Hi there! I went nuclear and just pulled the latest and added the changes I wanted to make.

The tests aren't passing but I think it's because the time zone offset is not empty when the tests are run. The test replaces GMT+0000 to UTC but expects GMT. I might be misunderstanding it though. If I have time, I'll try to make another PR with changes to the tests.

@matthewbadeau
Copy link
Author

Sorry for the multiple comments.

The tests are currently passing but it still doesn't fix the issue where 4 character time zones are not being displayed. For example, running LC_ALL="en_AU" TZ="Australia/Hobart" npm test fails. Running LC_ALL="en_US" TZ="Asia/Tokyo" npm test passes.

Before merging I will investigate more.

@mikegreiling
Copy link
Contributor

@chase-manning is there anything that can be done to move this PR forward?

@chase-manning
Copy link
Collaborator

@chase-manning is there anything that can be done to move this PR forward?

@mikegreiling It currently has a merge conflict, once that is fixed we can merge it 😄

@milohax milohax mentioned this pull request Sep 6, 2021
@mikegreiling
Copy link
Contributor

@chase-manning it looks like @milohax created a MR to address the conflicts with #165

@chase-manning
Copy link
Collaborator

Closing as this was completed in a seperate PR

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

Successfully merging this pull request may close these issues.

None yet

3 participants