Skip to content
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

Add lib and grpc time data types and conversions to enable future NI-DAQmx time support #449

Merged
merged 17 commits into from
Aug 31, 2023

Conversation

zhindes
Copy link
Collaborator

@zhindes zhindes commented Aug 21, 2023

What does this Pull Request accomplish?

It proposes one new data type:

  • nidaqmx._lib_time.AbsoluteTime implements NI-BTF and conversion to and from Python's datetime and NI's hightime (see hightime) types

It also provides conversions to and from gRPCs Timestamp type:

  • nidaqmx._grpc_time.convert_time_to_timestamp and nidaqmx._grpc_time.convert_timestamp_to_time provide conversion to and from hightime type (in addition to maintaining datetime support)

The proposal is that our user-facing API would all use datetime or hightime interchangeably, and we would use these types under-the-hood to call into the C API or gRPC. One minor issue I see:

  • Non-UTC timezones as out-params is something to think about. I think only returning UTC is good enough, but we could also add in-params that tell us what timezone to use. That doesn't work well with attributes, though, so my hunch is to stick with UTC-only as out-params and the user can convert.
  • hightime as out-params. I think its probably fine to only return hightime since they inherit from datetime, so it all works out.

Why should this Pull Request be merged?

Trying to get some base functionality in place so future PRs can start adding time support.

What testing has been done?

New component tests.

src/handwritten/time.py Outdated Show resolved Hide resolved
tests/component/test_time.py Outdated Show resolved Hide resolved
@zhindes zhindes changed the title Add AbsoluteTime data type to enable future NI-DAQmx time support Add lib and grpc time data types and conversions to enable future NI-DAQmx time support Aug 23, 2023
@github-actions
Copy link

github-actions bot commented Aug 23, 2023

Test Results

       30 files  ±       0         30 suites  ±0   44m 15s ⏱️ + 6m 39s
  1 206 tests +     64    1 045 ✔️ +     64     161 💤 ±0  0 ±0 
18 880 runs  +1 560  16 450 ✔️ +1 560  2 430 💤 ±0  0 ±0 

Results for commit 5c42903. ± Comparison against base commit f9e1a48.

♻️ This comment has been updated with latest results.

src/handwritten/_grpc_time.py Outdated Show resolved Hide resolved
generated/nidaqmx/_grpc_time.py Outdated Show resolved Hide resolved
poetry.lock Outdated Show resolved Hide resolved
tests/component/test_time.py Outdated Show resolved Hide resolved
tests/component/test_time.py Outdated Show resolved Hide resolved
tests/component/test_time.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@bkeryan bkeryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The important remaining issue is whether we should be converting to/from google.protobuf.timestamp_pb2.Timestamp or google.protobuf.internal.well_known_types.Timestamp.

These two new issues are not that important.

tests/unit/_time_utils.py Outdated Show resolved Hide resolved
src/handwritten/_grpc_time.py Outdated Show resolved Hide resolved
tests/component/test_time.py Outdated Show resolved Hide resolved
src/handwritten/_lib_time.py Outdated Show resolved Hide resolved
src/handwritten/_lib_time.py Show resolved Hide resolved
src/handwritten/_lib_time.py Outdated Show resolved Hide resolved
@zhindes zhindes merged commit 3b0faac into master Aug 31, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants