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

Patch Time.now with zone #783

Closed
wants to merge 2 commits into from

Conversation

@ScruffR
Copy link
Contributor

commented Dec 24, 2015

Since Time.now() always returns UTC/GMT irrespective of the set Time.zone() I thought it might be useful to add an easy way to have Time.now() return epoch time either with or without (=default) the offset.
static time_t now(bool withZone = false);
Time.now() will behave as always
Time.now(false) returns the same
Time.now(true) returns the epoch time + timeoffset in seconds

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Jan 1, 2016

Looks good. I wonder if we should have a separate method to retrieve the local time vs UTC time. e.g.

Time.local();
@ScruffR

This comment has been minimized.

Copy link
Contributor Author

commented Jan 1, 2016

I was thinking of that too, but couldn't decide on a name, but local() seems fine to me.
I thought of Time.nowLocal() vs. Time.nowWithZone() vs. Time.nowTimezoned() - thinking too complicated 😜

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2016

Would be much appreciated if you could also add API tests in user/tests/wiring/api for the new overloaded function, and a test case in user/test/wiring/no_fixture. You'll see we already have tests for the time API there.

@jenesaisdiq Any thoughts on this proposed API:

uint32_t utc_time = Time.now();  
// time is current UTC time - this is what we presently have

// proposed Time.local() API
Time.zone(-5);
uint32_t  local_time = Time.local();
// local_time will be 5 hours earlier than utc_time
@jenesaisdiq

This comment has been minimized.

Copy link

commented Jan 12, 2016

Sounds fine to me!

@bkobkobko

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2016

I like this too. This will help with the ported TImeAlarms since I had to peek under the hood and get the current timezone in the current version.

@m-mcgowan m-mcgowan added this to the 0.4.9 milestone Jan 12, 2016

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2016

To fetch the current timezone offset, we could also add Time.zone() - with 0 arguments it returns the current timezone:

Time.zone(5);   // +5 hours
float tz = Time.zone();
// tz == 5.0
@avtolstoy

This comment has been minimized.

Copy link
Member

commented Jan 13, 2016

@m-mcgowan Wouldn't TZ offset in seconds work better? Much like tm_gmtoff in struct time.

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2016

Possibly, but the Time.zone(float hours) API already exists, setting the precedent. I'm guessing the rationale is that expressing zone offsets in hours is conceptually simpler since timezones are commonly described in hours difference.

@avtolstoy

This comment has been minimized.

Copy link
Member

commented Jan 13, 2016

@m-mcgowan The world is big and there are apparently some half hour or even 45-minute timezones out there.

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2016

That's right, consequently Time.zone() takes a float, so fractional hours can be specified.

@ScruffR

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2016

@avtolstoy, there are even some with 45 min offsets, but I second Mat here with the use of float since the standardized way to denote timezones is as hours (and in the odd cases hh:mm).
So if we wanted to ríd the user of the need to think ;-) even more than with float I'd only add an overload Time.zone(int hh, int mm), but having Time.zone(int ss) seems rather prone to error - and it looks odd and more complicated (e.g Time.zone(44820) vs. Time.zone(+12.75) for Chatham Island (+12:45))


@m-mcgowan: Was the comment about adding a test meant for me?
If so, I'm happy to PR it.

m-mcgowan added a commit that referenced this pull request Jan 14, 2016
`Time.zone()` and `Time.local()` APIs to fetch the current timezone a…
…nd the current local time. Inspired by #783 - thanks @ScruffR
@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2016

Hi @ScruffR I didn't see your latest comment before had already coded up the tests, and the new APIs discussed. Thanks for this PR and the prompt to add local time support.

@m-mcgowan m-mcgowan closed this Jan 14, 2016

@ScruffR

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2016

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.