Repository navigation
Allow hightime_datetime_to_protobuf to accept non utc timezones - #395
mjohanse-emr wants to merge 4 commits into
Conversation
Signed-off-by: Michael Johansen <michael.johansen@emerson.com>
There was a problem hiding this comment.
🟡 Changes recommended
Naïve datetimes are host-dependent, and the test does not exercise a non-UTC timezone.
2 open findings
What changed in this PR
Normalizes hightime.datetime values to UTC before protobuf conversion.
Changes:
- Adds UTC normalization before conversion.
- Updates conversion unit coverage.
| File | Description |
|---|---|
precision_timestamp_conversion.py |
Adds UTC normalization. |
test_precision_timestamp_conversion.py |
Updates timestamp conversion test. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| def test___hightime_datetime___convert___valid_precision_timestamp() -> None: | ||
| ht_datetime = ht.datetime(year=2020, month=1, day=1, hour=5, minute=26, tzinfo=dt.timezone.utc) | ||
| ht_datetime = ht.datetime(year=2020, month=1, day=1, hour=5, minute=26) |
There was a problem hiding this comment.
Testing a single naive timestamp is inadequate. Please test:
- Dates before 1904
- Dates between 1904 and 1970
- Dates after 1970
- Fixed-offset timezones
- Real timezones like
ZoneInfo("America/Chicago"). Add a Windows-only test dependency for this:tzdata = { version = "*", markers = "sys_platform == 'win32'" } - The local timezone returned from the
tzlocalpackage.
There was a problem hiding this comment.
- The NI-BTF epoch (NI-DAQmx uses this as a sentinel value to denote unset timestamps)
There was a problem hiding this comment.
There should now be adequate testing. For the NI-BTF epoch, the test specifies the epoch as UTC, so it skips the actual conversion. If there's another way I should specify the epoch, please let me know. Perhaps adding a timezone offset and the corresponding hour value when creating the datetime?
…datetimes. Add more unit testing. Signed-off-by: Michael Johansen <michael.johansen@emerson.com>
Signed-off-by: Michael Johansen <michael.johansen@emerson.com>
Signed-off-by: Michael Johansen <michael.johansen@emerson.com>
There was a problem hiding this comment.
Why are you fixing this in ni.protobuf.types?
nitypes is where the conversion between bintime and hightime lives.
nidatastore is the API that needs this functionality.
Are waveform timestamps the reason you want to do this in a lower-level component rather than nidatastore?
There was a problem hiding this comment.
I based the location of my fix on @csjall 's comment here:
ni/datastore-python#116 (comment)
The main reason I didn't make the change in nidatastore was to reduce code duplication. Waveform timestamps weren't a major factor in this decision.
I didn't consider making the change in nitypes. If that is the better place to make this change, I can abandon this PR.
What does this Pull Request accomplish?
Adds a conversion to
datetime.timezone.utcinhightime_datetime_to_protobufto allow clients to pass non-utc timestamps to test result and step create methods.A new helper method
_hightime_datetime_to_utchas been added to handle pre-1970 values which are not supported byastimezone.Why should this Pull Request be merged?
Fixes
What testing has been done?