Correct parsing of DateTime in non-utc client timezone #83
Conversation
adamlofts
changed the title
| /// See: https://github.com/adamlofts/mysql1_dart/issues/39 | ||
| static DateTime parseDateTimeInUtc(String s) { | ||
| var lt = DateTime.parse(s); // lt is local time. | ||
| return DateTime.utc( |
There was a problem hiding this comment.
why not just return lt.add(lt.timeZoneOffset).toUtc();?
There was a problem hiding this comment.
I find this expression a bit cryptic. Its not obviously correct to me at a glance. For example its not obvious to me that we should add the offset and not subtract it. Its not clearly more performant so I went for the other version.
| /// Dart does not provide a simple way to do this. | ||
| /// See: https://github.com/adamlofts/mysql1_dart/issues/39 | ||
| static DateTime parseDateTimeInUtc(String s) { | ||
| var lt = DateTime.parse(s); // lt is local time. |
There was a problem hiding this comment.
instead of creating a variable called lt then adding a comment explaining what it means, why not just create a variable called localDateTime?
| @@ -63,7 +81,7 @@ class StandardDataPacket extends ResultRow { | |||
| case FIELD_TYPE_DATETIME: // datetime | |||
| case FIELD_TYPE_TIMESTAMP: // timestamp | |||
| var s = utf8.decode(list); | |||
| return DateTime.parse(s).toUtc(); | |||
| return parseDateTimeInUtc(s); | |||
There was a problem hiding this comment.
[Opinion] Why is this functioned off? I don't personally think the implementation is that complicated (especially if you simplify the function), the function isn't re-used and its name doesn't really add value to what's going on under the hood. I think you should probably just inline the code.
There was a problem hiding this comment.
Because you can understand what is happening from just the function name. Otherwise we would need a comment to explain it.
Fix: #39
Fix: #71
Fix: #61