rpgmaker / NetJSON

Faster than Any Binary? Benchmark: http://theburningmonk.com/2014/08/json-serializers-benchmarks-updated-2/
MIT License
230 stars 29 forks source link

VS2017. Fix for DateTime serialization with non-UTC timezone #191

Closed AlexSeleznyov closed 3 years ago

AlexSeleznyov commented 6 years ago

Migration to VS2017:

rpgmaker commented 6 years ago

Thanks for the PR. I will review it and if everything works then i will merge it. :)

rpgmaker commented 6 years ago

I have made the changes to the master branch. So you can rebase if you want to now.

Thanks again for the contribution

rpgmaker commented 6 years ago

Any progress on the datetime fixes?

Thanks,

AlexSeleznyov commented 6 years ago

Sorry but no :( The project I needed JSON library for is not of high priority anymore thus I had to postpone working on the fix as I am required elsewhere. I'll try to find some time next week.

rpgmaker commented 6 years ago

Thanks for the response, you can take your time. The part is that I have most of your changes from this pr already. So the date fix can take as long as it needs.

Thanks,

rpgmaker commented 6 years ago

Any update on this pr?

rpgmaker commented 6 years ago

The current changes in the fix do not require changes to unit tests? Right?

Thanks,

AlexSeleznyov commented 6 years ago

I've rebased to current source code then applied my changes - tests pass just fine https://ci.appveyor.com/project/AlexSeleznyov/netjson I use the updated code for a few months already and no issues were reported. Without these changes same test fails failingtest

rpgmaker commented 6 years ago

Rather than changing the test. Why not try to use the netjson settings NetJSON.TimeZoneFormat with local as the enumeration? And not make direct changes to the code for the date itself both in the test and the conversion method of date

Thanks,

rpgmaker commented 6 years ago

I am suggesting passing setting to thoses tests that need local time. And removing code changes on the logic for datetime in the core code itself to see if it resolve the problem

AlexSeleznyov commented 6 years ago

Actually, they do, as when run in non-GMT timezone, due to DateTime's Equal implementation, same timestamp in different representations is different from itself. For example, TestSerializeDateNowWithMillisecondDefaultFormatUtc has date in Msk timezone but ddate is in UTC thus comparison fails. There should be ether UtcNow or ToLocalTime for ddate to make sure they are of same kind.

AlexSeleznyov commented 6 years ago

Yes, I do use TimeZoneFormat with local and it works just fine until there is UTC datetime kind is used.

AlexSeleznyov commented 6 years ago

To get the same errors as I do, add this to AppVeyor's Environment Init script: Set-TimeZone -Id "Russian Standard Time"

rpgmaker commented 6 years ago

I see. I will look more into it. I guess using a different time zone expose the problem. Wanted minimum changes without breaking existing code

Thanks,

AlexSeleznyov commented 6 years ago

Can't agree more! Timezone is a heck of an issue - https://infiniteundo.com/post/25509354022/more-falsehoods-programmers-believe-about-time

rpgmaker commented 5 years ago

Any possibility that there is more work that needs to be done for this?

rpgmaker commented 3 years ago

I am going to close this PR since most of the changes are already in place. Re-open if needed.