Skip to content

Conversation

@moe-ad
Copy link
Contributor

@moe-ad moe-ad commented Nov 4, 2025

Related to #2695. It is a short change, but I want to explain the "why" clearly for future reference.

Recent studying of python type system and playing around the codebase made me realize a couple of things that are still wrong with the existing type propagation:

  • A custom collection like StringFieldsCollection is a actually recognized by type checkers as a collection of class objects rather than class instances. For example, this is why string_field.get_entity_data() below is expecting two arguments (the instance and the index) instead of one (the index):
    image
  • Type propagation does not even work at all on other IDEs like vscode (I tested fix: design and type hint of custom collections #2695 on PyCharm alone 😞)
    image
    image
    image

In summary, turns out:

  • The second problem is caused by not correctly identifying the subtype parameter of collection_factory method as what actually drives the type parameterization of that method. This is solved by using a type parameter specific to the method. See here.
  • The first problem is solved annotating subtype with Type[Type Param] rather than just Type Param. Doing that ensures Collection[Type Param] in the return annotation is correctly interpreted as a collection of instances and not of class objects.

With this change, things work accurately now across both PyCharm and vscode.

@moe-ad moe-ad self-assigned this Nov 4, 2025
@moe-ad moe-ad requested a review from PProfizi November 4, 2025 13:31
Copy link
Contributor

@PProfizi PProfizi left a comment

Choose a reason for hiding this comment

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

That's awesome @moe-ad! Thanks for that!

@codecov
Copy link

codecov bot commented Nov 4, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.46%. Comparing base (00d243b) to head (e551960).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2724      +/-   ##
==========================================
+ Coverage   84.38%   84.46%   +0.08%     
==========================================
  Files          92       92              
  Lines       10899    10900       +1     
==========================================
+ Hits         9197     9207      +10     
+ Misses       1702     1693       -9     

@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2025

Some tests with 'continue-on-error: true' have failed:

  • PyDPF-Post API tests on ubuntu-latest

Created by continue-on-error-comment

@moe-ad moe-ad enabled auto-merge (squash) November 4, 2025 14:50
@moe-ad moe-ad merged commit 18d569c into main Nov 4, 2025
45 checks passed
@moe-ad moe-ad deleted the fix/typing-of-collections branch November 4, 2025 15:24
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.

3 participants