add datetime support #569
add datetime support #569
Conversation
|
Why not just continue with #519? |
|
@nielsenko its not based on master |
|
We could have just rebased on #519 on master but I guess it doesn't matter that much. |
| throw Exception("Not implemented"); | ||
| final seconds = ref.values.timestamp.seconds; | ||
| final nanoseconds = ref.values.timestamp.nanoseconds; | ||
| return DateTime.fromMicrosecondsSinceEpoch(seconds * _microsecondsPerSecond + nanoseconds ~/ _nanosecondsPerMicrosecond, isUtc: true); |
There was a problem hiding this comment.
I think there should be .toLocal() at the end. When we pass the value to core we convert it toUtc. Here we are receiving utc from core and we have to convert it back to local before to return.
There was a problem hiding this comment.
I don't think we should be converting dates to local because Core doesn't store timezone info. It'll be misleading for developers if we returned local date here. Also, there's no guarantee that the original date the developer passed was in local time - it could be UTC or some completely different timezone.
There was a problem hiding this comment.
It is not needed the core to keep time zone. if you pass a local date from one device, then when you convert toUtc the method will use the current time zone of the device. And when they receive it on another device it will be converted to the device local time.
In case we want the users to manage their dates then we shouldn't convert their dates to UTC in _intoRealmValue method without they to know. Probably have to throw if (value as DateTime).isUtc==false before to change their date.
There was a problem hiding this comment.
I think it's a little annoying if we force people to always convert dates to utc although I agree it's more correct as we explicitly tell them that Realm will lose the timezone information.
There was a problem hiding this comment.
When they create a DateTime in Dart it is local by default unless they have used DateTime.utc constructor. It will be good to have at least some warning or api doc.
There was a problem hiding this comment.
We should definitely document it in the main docs site. We could also generate an API doc on the user model property, though not sure how intrusive that will be.
|
@desistefanova @nirinchev Could you have your say on this so we could merge it. I see some comments but I am unsure if I need to take some action on the code. If there are changes needed please write a clear comment on that ? |
|
I wrote the code so I'm not sure it's me who should comment on whether to merge it or not. |
@blagoev could we discuss it? I will add this to the list. |
|
sure let's discuss this. |
|
Needs a rebase though.. |
# Conflicts: # CHANGELOG.md # test/test.dart # test/test.g.dart
supersedes #519