Skip to content

Conversation

@mjohanse-emr
Copy link
Contributor

@mjohanse-emr mjohanse-emr commented Aug 3, 2025

What does this Pull Request accomplish?

This change adds the module name to the python type name string that is used when determining what converter to use based on the python type.

This makes it so that the grpc conversion code can tell the difference between the different datetime and timedelta types:
bintime.DateTime, hightime.datetime, datetime.datetime
bintime.TimeDelta, hightime.timedelta, datetime.timedelta

To do this, I created a new method called get_candidate_strings which takes the list of "underlying parents" and returns the disambiguated type strings for those types. Meaning that they contain the module name as well as the type name.

I've updated all converters to specify their type string as module.type. Example: builtins.int
I've updated all collection converters to specify their type string to be like collections.abc.Collection[builtins.int]

I refactored the Converter class into three separate classes to handle the type naming in the base classes:

  • Converter
  • CollectionConverter
  • CollectionConverter2D

Why should this Pull Request be merged?

So that the conversion code can tell the difference between different timing types and convert them appropriately.
Implements part of:

What testing has been done?

I updated the converter unit tests, which are passing. The rest of the unit tests pass as well. Also mypy, pyright, styleguide.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2025

Test Results

   10 files  ± 0     10 suites  ±0   24s ⏱️ -1s
  228 tests + 1    228 ✅ + 1  0 💤 ±0  0 ❌ ±0 
2 230 runs  +10  2 230 ✅ +10  0 💤 ±0  0 ❌ ±0 

Results for commit 1a61b7e. ± Comparison against base commit b5f258f.

This pull request removes 52 and adds 53 tests. Note that renamed tests count towards both.
tests.unit.test_convert ‑ test___various_python_objects___get_best_matching_type___returns_correct_type_string[1-int]
tests.unit.test_convert ‑ test___various_python_objects___get_best_matching_type___returns_correct_type_string[10-int]
tests.unit.test_convert ‑ test___various_python_objects___get_best_matching_type___returns_correct_type_string[123-int]
tests.unit.test_convert ‑ test___various_python_objects___get_best_matching_type___returns_correct_type_string[456.2-float]
tests.unit.test_convert ‑ test___various_python_objects___get_best_matching_type___returns_correct_type_string[False-bool]
tests.unit.test_convert ‑ test___various_python_objects___get_best_matching_type___returns_correct_type_string[MixinIntEnum.VALUE11-int]
tests.unit.test_convert ‑ test___various_python_objects___get_best_matching_type___returns_correct_type_string[MyIntEnum.VALUE10-int]
tests.unit.test_convert ‑ test___various_python_objects___get_best_matching_type___returns_correct_type_string[MyIntFlags.VALUE1-int]
tests.unit.test_convert ‑ test___various_python_objects___get_best_matching_type___returns_correct_type_string[mystr-bytes]
tests.unit.test_convert ‑ test___various_python_objects___get_best_matching_type___returns_correct_type_string[mystr-str]
…
tests.unit.test_convert ‑ test___various_python_objects___get_best_matching_type___returns_correct_type_string[1-builtins.int]
tests.unit.test_convert ‑ test___various_python_objects___get_best_matching_type___returns_correct_type_string[10-builtins.int]
tests.unit.test_convert ‑ test___various_python_objects___get_best_matching_type___returns_correct_type_string[123-builtins.int]
tests.unit.test_convert ‑ test___various_python_objects___get_best_matching_type___returns_correct_type_string[456.2-builtins.float]
tests.unit.test_convert ‑ test___various_python_objects___get_best_matching_type___returns_correct_type_string[False-builtins.bool]
tests.unit.test_convert ‑ test___various_python_objects___get_best_matching_type___returns_correct_type_string[MixinIntEnum.VALUE11-builtins.int]
tests.unit.test_convert ‑ test___various_python_objects___get_best_matching_type___returns_correct_type_string[MyIntEnum.VALUE10-builtins.int]
tests.unit.test_convert ‑ test___various_python_objects___get_best_matching_type___returns_correct_type_string[MyIntFlags.VALUE1-builtins.int]
tests.unit.test_convert ‑ test___various_python_objects___get_best_matching_type___returns_correct_type_string[mystr-builtins.bytes]
tests.unit.test_convert ‑ test___various_python_objects___get_best_matching_type___returns_correct_type_string[mystr-builtins.str]
…

♻️ This comment has been updated with latest results.

Copy link
Contributor Author

@mjohanse-emr mjohanse-emr left a comment

Choose a reason for hiding this comment

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

This PR has a logical conflict with #128. Whichever PR gets in second will have to update the timedelta converter to return the type string datetime.timedelta.

Copy link
Collaborator

@jfriedri-ni jfriedri-ni left a comment

Choose a reason for hiding this comment

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

I would like us to strongly consider changing python_typename to always use '{module}.{class}' for every converter.

Michael Johansen added 2 commits August 4, 2025 14:04
@mjohanse-emr mjohanse-emr changed the title Disambiguate datetime and timedelta types when converting. Include module name in python type name strings to disambiguate types when converting. Aug 4, 2025
…or Collection. Type name logic moved into base class.

Signed-off-by: Michael Johansen <[email protected]>
@mjohanse-emr mjohanse-emr requested a review from bkeryan August 7, 2025 15:25
@mjohanse-emr mjohanse-emr merged commit 529e28d into main Aug 8, 2025
14 checks passed
@mjohanse-emr mjohanse-emr deleted the users/mjohanse/disambiguate_time_types branch August 8, 2025 19:07
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.

6 participants