Skip to content

Conversation

edan-bainglass
Copy link
Member

@edan-bainglass edan-bainglass commented Sep 28, 2025

This PR updates type hinting in the ORM module. Mostly, it's a matter of importing future annotations and updating to 3.10 standards. Self is added to aiida.common.typing, where sys is used to determine from where to import Self. Self is introduced to improve typing for entity collections, which in turn makes CollectionType redundant, which is removed.

Minimal formatting updates here and there.

Update

Self is introduced to improve typing for entity collections, which in turn makes CollectionType redundant, which is removed.

This proved to be more difficult. Reverting for now. I suspect there may be an issue with the @classproperty decorator that is interfering with mypy.

@edan-bainglass
Copy link
Member Author

Oh boy. Misunderstood the function of from __future__ import annotations. Okay, this PR won't work in 3.9. Closing for now, but good to have as a reference, since we're considering dropping 3.9 support soon.

@edan-bainglass
Copy link
Member Author

edan-bainglass commented Sep 28, 2025

Reopening, as much of this can still hold. Just not | operations for Optional and Union, which is fine. I'll revert this (stash for later).

Update

Actually, | works for type hints. Just not for type definition, so no NewType = int | str allowed. Easy!

@edan-bainglass edan-bainglass force-pushed the update-orm-typing branch 2 times, most recently from 18bc4d3 to 32a4456 Compare September 28, 2025 15:00
Copy link

codecov bot commented Sep 28, 2025

Codecov Report

❌ Patch coverage is 99.02200% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.26%. Comparing base (ebc9d4a) to head (bbd8a13).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
src/aiida/cmdline/utils/common.py 71.43% 2 Missing ⚠️
src/aiida/common/typing.py 80.00% 1 Missing ⚠️
src/aiida/orm/entities.py 95.84% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7026      +/-   ##
==========================================
- Coverage   79.27%   79.26%   -0.00%     
==========================================
  Files         566      566              
  Lines       43779    43805      +26     
==========================================
+ Hits        34700    34717      +17     
- Misses       9079     9088       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@edan-bainglass edan-bainglass force-pushed the update-orm-typing branch 3 times, most recently from 33d343d to 1901a58 Compare September 28, 2025 16:47
@danielhollas
Copy link
Collaborator

@edan-bainglass great to see work on typing. :-)

It seems that this partly overlaps with my work in #7019 and #6704. Would be great if you could have a look and review. Especially #6704 should go in first because some of the files that you're modifying here are not actually type-checked in pre-commit.

@edan-bainglass
Copy link
Member Author

@edan-bainglass great to see work on typing. :-)

It seems that this partly overlaps with my work in #7019 and #6704. Would be great if you could have a look and review. Especially #6704 should go in first because some of the files that you're modifying here are not actually type-checked in pre-commit.

Okay cool. I'll have a look this week.

@edan-bainglass
Copy link
Member Author

@danielhollas honest opinion, is this PR required? I believe you stated this type of work can be automated by ruff, once some prelim work is done, which you're addressing in recent PRs. Is this accurate?

@danielhollas
Copy link
Collaborator

@danielhollas honest opinion, is this PR required?

Hah, thanks, I was going to write the same thing here but was procrastinating on it as I didn't want to be a PITA. 😅

I don't think we should merge this at this point: we should wait until we drop Python 3.9, and then convert everything over whole codebase at once with ruff. Then we'll add the resulting commit to .git-blame-ignore-revs to preserve git history.

@edan-bainglass
Copy link
Member Author

Hah, thanks, I was going to write the same thing here but was procrastinating on it as I didn't want to be a PITA. 😅

Not at all. I rather close it and have the robots handle it cleanly.

I don't think we should merge this at this point: we should wait until we drop Python 3.9, and then convert everything over whole codebase at once with ruff. Then we'll add the resulting commit to .git-blame-ignore-revs to preserve git history.

Sounds like a plan 👍 We probably should look more seriously into doing 3.9 in AiiDAlab

@danielhollas
Copy link
Collaborator

We probably should look more seriously into doing 3.9 in AiiDAlab

Yep, see aiidalab/aiidalab-docker-stack#455 (comment)

@edan-bainglass
Copy link
Member Author

Most of what is done here is already accomplished by a series of recent PRs by @danielhollas. As suggested above, once we drop support for Python 3.9, we will push one commit to update all type hints across the codebase. As such, this PR is no longer necessary. Closing...

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