-
Notifications
You must be signed in to change notification settings - Fork 603
Option to convert dates (also convert back to ISO in ParseJson) #205
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
base: master
Are you sure you want to change the base?
Conversation
Added an additional user option that lets the user choose if dates should be converted to UTC. Right now, dates are only converted when calling JsonConverter.ConvertToJSON. But not when JsonConverter.ParseJson is called. In addition to the my previous pull request, which fixes the latter, this adds an option to turn it on for both (default, false) or keep the date times as they are.
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 forgot to add JsonOptions where the new boolean option is used. Should be JsonOptions.DontConvertDates
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.
Some comments IMHO:
ConvertToJson
Case VBA.vbDate
' Date
If Not DontConvertDates Then
json_DateStr = ConvertToIso(VBA.CDate(JsonValue))
Else
json_DateStr = VBA.CDate(JsonValue)
End If
ConvertToJson = """" & json_DateStr & """"
- The
CDate
calls (also in the base version) are redundant. ThisCase
section is specifically forDate
data type. - The new option will convert
JsonValue
to a local (regionalised) date string such as “05/26/2022 6:16:10 PM”, which is not at all ISO8601-compliant and won’t be recognised byParseJSON
if reloaded. - The
Date
data type is a 64-bit floating point number. So it has to be converted to a string representing a portable date-time. ISO8601 strings fit the bill. - An alternative to the ‘Z’ string would be one showing local time with the time zone offset, e.g. ”26-05-2022T18:16:10+01:00” (slightly longer than necessary but acceptable to the current
ParseISO
). That option would be a change toConvertToIso
in VBA-UTC. - If you really want the regionalised date, you should add it as a separate
String
field to your data structure before callingConvertToJson.
ParseJSON
I agree with the principle that, within reason, reloaded data structures (byParseJSON
) should be close to the ones exported (byConvertToJson
). The current version reloads dates to ‘Z’ ISO strings. But we need to be careful about JSON objects (Dictionary
) since keys areString.
I would prefer to keep incoming ISO8601 keys intact, especially if we add millisecond support to VBA-UTC.
If Not DontConvertDates Then _
If json_ParseString Like "####-##-##T##:##:##.###Z" Then _
json_ParseString = ParseIso(json_ParseString)
json_ParseString
returns aString.
So theDate
fromParseISO
will be reconverted to a regionalised dateString.
not aDate.
- The recognition test is a bit restrictive and won’t allow other reasonable ISO8601 strings from external sources (or changes to
ConvertToIso
). - To convert values but not keys, this code should be moved into
json_ParseValue
just after thejson_ParseString
call. - I would prefer a positive option name: LocaliseISOdates ?
So something like:
Case """", "'"
json_ParseValue = json_ParseString(json_String, json_Index)
If JsonOptions.LocaliseISOdates And json_ParseValue Like "####-##-##T##:##*" Then
json_ParseValue = ParseIso(json_ParseValue & "")
End If
Hope that helps.
I’m a GitHub newbie. This may have been the wrong place to post. Apologies if so.
FWIW, using a negative in this case allows for it to be the default action because of how |
Added an additional user option that lets the user choose if dates should be converted to UTC. Right now, dates are only converted when calling JsonConverter.ConvertToJSON. But not when JsonConverter.ParseJson is called. In addition to the my previous pull request, which fixes the latter, this adds an option to turn it on for both (default, false) or keep the date times as they are.