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

Added an option to display UTC time instead of local time #38

Merged
merged 7 commits into from
Jul 9, 2017
Merged

Added an option to display UTC time instead of local time #38

merged 7 commits into from
Jul 9, 2017

Conversation

ftm
Copy link
Contributor

@ftm ftm commented Jul 5, 2017

As put forward by @gwax in #30, this adds an option to display the time as UTC time instead of local time. I've included some specs as well as the actual functionality

screen shot 2017-07-05 at 20 07 13
screen shot 2017-07-05 at 20 07 21

@ftm
Copy link
Contributor Author

ftm commented Jul 5, 2017

Oops, the tests I wrote rely on the system running them to have a UTC offset of +0100 so the local time displays correctly. I'll work on this more.

@ftm
Copy link
Contributor Author

ftm commented Jul 5, 2017

The latest two versions actually pass the tests now, just thought I'd give some insight into what was causing the problem.

In order to test whether UTC mode worked I wanted to try and change the timezone. Mockdate allows you to set the timezone offset using its second argument, which I expected to be akin to changing the entire system timezone, however it seems to only alter the value returned by new Date().getTimezoneOffset(). This leads to a confusing scenario as the value returned by new Date().toString() uses the new mock date/time but still uses the original system timezone (i.e. GMT+0000 (UTC) on build servers but GMT+0100 (BST) for me) not the value set using timezoneOffset.

mockdate.set('1987-04-11 04:00', -300)

// On Build Server
new Date().toString() // Sat Apr 11 1987 04:00:00 GMT+0000 (UTC)
new Date().getTimezoneOffset() // -300

// On Local Machine
new Date().toString() // Sat Apr 11 1987 04:00:00 GMT+0100 (BST)
new Date().getTimezoneOffset() // -300

This didn't seem to play nice with the way moment appears handles timezones/offsets. Moment defines two modes, local and UTC. Local is what is used by default or after .local() is called, and UTC is what is used after .utc() is called. Say MockDate.set('1987-04-11 04:00', -300) is used in the test: in local mode, the MockDate seems to work with moment().format('H ZZ') returning 4 +0500 no matter what timezone the system is running on. However in UTC mode things aren't as straightforward, the value returned will be H +0000 where H varies depending on the timezone of the system irregardless of the value of the timezoneOffset. So on the build system where the timezone is +0000, H remains as 4 - but on my system where the timezone is +0100, H changes to 3 as it thinks my system is one hour ahead and corrects for it by decreasing the hour by 1.

mockdate.set('1987-04-11 04:00', -300)

// On Build Server
moment().format('H ZZ') // 4 +0500
moment().utc().format('H ZZ') // 4 +0000

// On Local Machine
moment().format('H ZZ') // 4 +0500
moment().utc().format('H ZZ') // 3 +0000

This inconsistency between the hours/H value and the timezone/ZZ on different system timezones is what confused me for so long as I was trying to test based on both. The new tests only pay attention to ZZ, setting it to +0500 using a timezoneOffset of -300, and testing for it to be +0000 in UTC mode. Testing based on the hours as well would require the test to adjust the expected values based on the system timezone which isn't really needed.

TL;DR: Inconsistencies in hours due to different system timezones played havoc with the tests so now they just ignore the hours

@ftm
Copy link
Contributor Author

ftm commented Jul 6, 2017

Latest push just adds a menu item and command to toggle the UTC mode so users can map a key to it if they want

@b3by
Copy link
Owner

b3by commented Jul 9, 2017

This is actually right on spot. It is a cool feature, I like it.
The PR seems ok. I'm merging it and bumping the package version. I'll actually make just a couple of small fixes here and there (probably only small typos).
Thank you for the hard work!

On a different note, I'll keep #30 still open, as it is originally related to a different feature (also nice, something I'll work on later this week).

Cheerio!

@b3by b3by merged commit beeb9cf into b3by:master Jul 9, 2017
@ftm ftm deleted the utc-option branch July 9, 2017 20:09
@gwax
Copy link

gwax commented Jul 20, 2017

This is working out great for me. Thanks.

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.

3 participants