search

coderaanalytics / econdatar

R package for uploading and downloading data to/from www.econdata.co.za
MIT License
6 stars 2 forks source link

`write_release(release=Sys.time())` assumes that all Codera data engineers will work in GMT+2 #17

Closed aidanhorn closed 7 months ago

aidanhorn commented 7 months ago

query_params$release <- format(Sys.time(), "%Y-%m-%dT%H:%M:%S") assumes that the data engineer or computer that loads data into EconData will be set at GMT+2. Please edit so that the time zone is taken into account.

As an aside, I see that release has a slightly different meaning to the old tstamp. I take it that we will try to have small differences in time between our suppliers' publication and our load, when we don't specify the release parameter in write_release() (auto-etl/utils/cli_args.R doesn't have a release parameter for the automation), but the definition of this timestamp is clearly different when we retrospectively set the parameter.

The else part of the definition of release could be treated as a failsafe if we record tabular metadata for the supplier release timestamps (which calendar-releases can help with).

byrongibby commented 7 months ago

I have changed the code to 

query_params$release <- format(Sys.time(),
  format = "%Y-%m-%dT%H:%M:%S",
  tz = "Africa/Johannesburg")

Which will account for the possibility that the person releasing the data is not using SAST.

aidanhorn commented 7 months ago

We discussed the second part of this issue in a video call now:

The Oversight Dashboard will be inaccurate if a release is delayed by more than the period (assuming the latest period is not released), and the release parameter is not specified manually.

aidanhorn commented 7 months ago

@byrongibby can you please open this issue again, because the timestamps in release for today were two hours ahead than what would have been expected (they were past 16:20 when I checked before it was 16:00, and our meeting was at 15:00). I tested

format(Sys.time(),
  format = "%Y-%m-%dT%H:%M:%S",
  tz = "Africa/Johannesburg")

in a terminal on my computer, and it seems to work fine, so @jacques-rs is the locale on your computer South Africa?

@jacques-rs please review my temporary branch auto-etl/tree/release and merge! My proposal provides some straightforward code to use the calendar start time as the release timestamp, so this whole issue of specifying the release parameter in auto-etl is automatically fixed.