iridium-toolkit icon indicating copy to clipboard operation
iridium-toolkit copied to clipboard

time as UTC, remove useless offset formatter

Open rpatel3001 opened this issue 1 year ago • 5 comments

Opening this to address an issue raised here

Two parts to this little change:

  1. remove the useless %z format spec. The exisiting fromtimestamp() call does not have a timezone specified, so the resulting datetime is naive, which means the %z specifier resolves to an empty string.
  2. Add the UTC timezone to the fromtimestamp call. Without this the datetime object represents the time in the users local timezone but the object does not have any indication of what timezone it represents.

I think @kevinelliott would probably also like this change, or at least be indifferent, as regards Airframes ingest.

rpatel3001 avatar Apr 18 '24 00:04 rpatel3001

The code was indeed intended to print the timezone offset. I somehow missed that it was missing. Your change seems to just change the times printed from local time to UTC without adding any indication that it is now different. I don't think that's a good idea.

What I intended was probably more like:

q.timestamp = datetime.datetime.fromtimestamp(q.time, datetime.timezone.utc).astimezone().strftime("%Y-%m-%dT%H:%M:%S%z")

while your change would probably better be reflected as:

q.timestamp = datetime.datetime.fromtimestamp(q.time, datetime.timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ")

I'm not really sure what the better option is in this case. I think a format change is necessary either way. Let me know if you have some arguments, anyway, I'll ponder this for a bit and commit a change probably next week.

Sec42 avatar Apr 18 '24 18:04 Sec42

@kevinelliott will definitely want to know about a format change. If it's going to change, my vote would be for just a raw seconds from epoch timestamp in the JSON output, and then apply whatever fix to display the timezone offset for printing to stdout.

rpatel3001 avatar Apr 18 '24 18:04 rpatel3001

any thoughts on this?

rpatel3001 avatar Jun 03 '24 00:06 rpatel3001

I think I fixed all the timezone bugs in 47377de5 -- except in the acars output as I haven't gotten any feedback there yet and didn't want to break existing consumers.

Sec42 avatar Jun 05 '24 18:06 Sec42

997ed97c on the testing branch is my suggestion for the corresponding acars change. If none of you: @kevinelliott @rpatel3001 raise any issues, I will move that to the master branch in the near future.

This changes the timestamp strings to contain a trailing "Z" to explicitly signal that they are in UTC, so existing parsing may need to be changed to accept them.

Sec42 avatar Jun 16 '24 12:06 Sec42

I think all cases are now fixed in master. Please let me know if something is still wrong.

Sec42 avatar Nov 17 '24 18:11 Sec42