-
Notifications
You must be signed in to change notification settings - Fork 89
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
support zoned datetime format #23
Conversation
8740469
to
f5963d6
Compare
encoder.go
Outdated
@@ -15,7 +16,8 @@ const ( | |||
) | |||
|
|||
var ( | |||
textEncode encoder = new(textEncoder) | |||
textEncode encoder = new(textEncoder) | |||
timeZoneRegexp = regexp.MustCompile("\\\\'.+\\\\'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please give an example, which string will be parsed by this pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like it will be faster to get text enclosed by '' via string.find and slicing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was to find substring of kind "'<TIME_ZONE_NAME>'" in string such as "DateTime('Europe/Moscow')".
Yes, you're right, usage of find operations is much faster and enables to remove unquoting below.
if t, err = time.ParseInLocation(timeFormat, unquote(v), loc); err != nil { | ||
return t, err | ||
} | ||
return t.In(d.location), nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe ParseInLocation
returns time in proper location, and it does not need to call method In
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like when loc
is location received from database, it might differ from the one set in driver (d.location
). So I thought in this case dateTime should be converted to the location set in driver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, Your are right. ok.
|
||
func TestDecodeTimeWithLocation(t *testing.T) { | ||
dt := time.Date(2011, 3, 6, 3, 20, 0, 0, time.UTC) | ||
dataType := "DateTime(\\'Europe/Moscow\\')" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which are different from test case in line 67 on this file ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In test case on line 67 database location is discarded (useLocation: false
on line 78). In this test location is taken into account and time is converted from Europe\Moscow to UTC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
the code is ok, please squash commits. |
conn.go
Outdated
@@ -45,7 +46,8 @@ func newConn(cfg *Config) *conn { | |||
IdleConnTimeout: cfg.IdleTimeout, | |||
ResponseHeaderTimeout: cfg.ReadTimeout, | |||
}, | |||
logger: logger, | |||
logger: logger, | |||
useDBLocation: cfg.UseDBLocation, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this closer to member location ?
location: cfg.Location,
useDBLocation: cfg.UserDBLocation,
LGTM, thank you for PR |
Add support of zoned datetime and recalculation of zoned time returned from database as a config option.