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

Parse dates in current Location? #49

Closed
Xfennec opened this issue Mar 10, 2017 · 3 comments
Closed

Parse dates in current Location? #49

Xfennec opened this issue Mar 10, 2017 · 3 comments
Assignees

Comments

@Xfennec
Copy link
Contributor

Xfennec commented Mar 10, 2017

Hi,
First, big thanks for this library, it's awesome, really.

I use custom functions in my expression returning (float64)xxx.Unix() values (I hope it's correct to do so?). But when comparing values with literal dates like '2017-03-11' (without an explicit zone) , I have timezone offset.

Currently, dates are parsed using time.Parse() and the documentation states "In the absence of a time zone indicator, Parse returns a time in UTC." It makes me think that time.ParseInLocation() with the current Location should be used instead.

What do you think? It sounds OK to you? If so, I can create a PR, of course.

Thanks again for the lib !

@Knetic
Copy link
Owner

Knetic commented Mar 11, 2017

Hey, thanks for using the library!

First, yes that's correct to return (float64)xxx.Unix(), the library handles time by converting to the float64 representation of "unix time".

And, yeah, i think you're probably right that ParseInLocation is the right way to do it. I hadn't given it much thought at the time, but I can't think of a reason why it shouldn't use the current location by default. Go ahead and make a PR! 👍

@Knetic Knetic self-assigned this Mar 11, 2017
@Xfennec
Copy link
Contributor Author

Xfennec commented Mar 12, 2017

Thanks for the answer. While thinking about this, I realize that it could be a good idea to let the user chose its setting about date parsing, that may be time.Local or any particular Location. It also allows the default to be time.UTC so it will (mostly) match the current behavior of the library and won't break compatibility.

If I try to had such thing, how do you see it? As a per-expression setting or a global one? What the API would look like? Any thoughts?

@Knetic
Copy link
Owner

Knetic commented Mar 13, 2017

It could be a good idea to have that kind of setting, but seems sort of edge-case. I wouldn't worry about it for now. If it becomes a problem, there are a couple ways to handle it later.

I'm not so concerned with compatibility, this actually feels like it borders on a bug. If i were a user, i would expect to be able to pass time.Now() as a parameter - which returns the current Location's time, which would lead to me comparing a time literal, parsed in UTC (because it uses time.Parse), against a parameter, which is in the current Location.

Parsing in the current location (unless explicitly specified in the time string) is also is what i think most users would expect.

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

No branches or pull requests

2 participants