Skip to content

Conversation

mjohanse-emr
Copy link
Contributor

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

What does this Pull Request accomplish?

Updates the gRPC conversion logic to determine if something is a "collection" by checking if it's an instance of collections.abc.Collection, but not an instance of several other types that are technically collections but are handled with a custom converter.

Exclusions:

  • str: Has a separate converter
  • bytes: Has a separate converter
  • enum.Enum: Converted as ints
  • dict: Not a valid type to convert
  • ni.protobuf.types.Vector: Has a separate converter

Why should this Pull Request be merged?

AB#3175632

What testing has been done?

Unit tests, mypy, pyright, styleguide

Previous RFC Comments

This is a Request for Comment (at least for now) PR addressing this research item about using collections.abc instead of list, set, frozen_set, etc. to determine collections for conversion.

AB#3175632

The question to answer here is whether the implementation in this PR is better and/or more maintainable than keeping a "inclusion" list of the types of collections we support. Note that I do not know if the exclusion list is complete, but it does satisfy the unit tests that were already written. We'll have to maintain the exclusion list, so we'll have to decide whether that's easier than maintaining the inclusion list.

Michael Johansen added 2 commits September 3, 2025 09:01
… of concrete collection types

Signed-off-by: Michael Johansen <michael.johansen@ni.com>
Signed-off-by: Michael Johansen <michael.johansen@ni.com>
Signed-off-by: Michael Johansen <michael.johansen@ni.com>
Copy link
Contributor

github-actions bot commented Sep 3, 2025

Test Results

   10 files  ±0     10 suites  ±0   22s ⏱️ -5s
  256 tests ±0    256 ✅ ±0  0 💤 ±0  0 ❌ ±0 
2 510 runs  ±0  2 510 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 7f08d0c. ± Comparison against base commit 6b261b5.

♻️ This comment has been updated with latest results.

Signed-off-by: Michael Johansen <michael.johansen@ni.com>
jfriedri-ni

This comment was marked as duplicate.

Michael Johansen added 2 commits September 4, 2025 14:57
Signed-off-by: Michael Johansen <michael.johansen@ni.com>
Signed-off-by: Michael Johansen <michael.johansen@ni.com>
@mjohanse-emr mjohanse-emr changed the title RFC: Type match with collections.abc.Collection and some exclusions Type match with collections.abc.Collection and some exclusions Sep 4, 2025
@mjohanse-emr mjohanse-emr merged commit ce64be8 into main Sep 4, 2025
14 checks passed
@mjohanse-emr mjohanse-emr deleted the users/mjohanse/use_collections branch September 4, 2025 20:10
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.

4 participants