muccc / iridium-toolkit

A set of tools to parse Iridium frames
483 stars 112 forks source link

time as UTC, remove useless offset formatter #116

Closed rpatel3001 closed 5 days ago

rpatel3001 commented 7 months ago

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.

Sec42 commented 7 months ago

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.

rpatel3001 commented 7 months ago

@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 commented 5 months ago

any thoughts on this?

Sec42 commented 5 months ago

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 commented 5 months ago

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 commented 5 days ago

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