-
Notifications
You must be signed in to change notification settings - Fork 231
Type check storage.psql_dos.orm.querybuilder.main module #7065
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
Conversation
else: | ||
if column is None: | ||
if (alias is None) and (column_name is None): | ||
if alias is None or column_name is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a logic bug, per the get_column
type signature, neither alias nor column name can be None.
return type_filter, casted_entity | ||
|
||
if column is None: | ||
if alias is None or column_name is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just added the same check as above, otherwise the type checker complains.
|
||
try: | ||
return self._backend.get_backend_entity(res) | ||
return self._backend.get_backend_entity(res) # type: ignore[attr-defined] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't spent too much time on these type ignores, since most storage
module is untyped currently, and just enabling time checking on this file was an easy win.
d19dbd4
to
4077ee5
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7065 +/- ##
==========================================
- Coverage 79.32% 79.30% -0.02%
==========================================
Files 566 566
Lines 43883 43887 +4
==========================================
- Hits 34807 34801 -6
- Misses 9076 9086 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Can you say more about how ty caught the bug?
Actually both ty and mypy see the issue, here's mypy diagnostic ❯ mypy --pretty storage/psql_dos/orm/querybuilder/main.py
storage/psql_dos/orm/querybuilder/main.py:599: error: Argument 1 to "get_column" has incompatible type "str | None"; expected
"str" [arg-type]
column = get_column(column_name, alias)
^~~~~~~~~~~
storage/psql_dos/orm/querybuilder/main.py:599: error: Argument 2 to "get_column" has incompatible type
"AliasedClass[Any] | None"; expected "AliasedClass[Any]" [arg-type]
column = get_column(column_name, alias)
^~~~~
Found 2 errors in 1 file (checked 1 source file) And here's ty ❯ ty check src/aiida/storage/psql_dos/orm/querybuilder/main.py
error[invalid-argument-type]: Argument to function `get_column` is incorrect
--> src/aiida/storage/psql_dos/orm/querybuilder/main.py:599:41
|
597 | if alias is None and column_name is None:
598 | raise RuntimeError('I need to get the column but do not know the alias and the column name')
599 | column = get_column(column_name, alias)
| ^^^^^^^^^^^ Expected `str`, found `str | None`
600 | expr = self.get_filter_expr_from_column(operator, value, column)
|
info: Function defined here
--> src/aiida/storage/psql_dos/orm/querybuilder/main.py:900:5
|
900 | def get_column(colname: str, alias: AliasedClass) -> InstrumentedAttribute:
| ^^^^^^^^^^ ------------ Parameter declared here
901 | """Return the column for a given projection."""
902 | table_name = get_table_name(alias)
|
info: rule `invalid-argument-type` is enabled by default
error[invalid-argument-type]: Argument to function `get_column` is incorrect
--> src/aiida/storage/psql_dos/orm/querybuilder/main.py:599:54
|
597 | if alias is None and column_name is None:
598 | raise RuntimeError('I need to get the column but do not know the alias and the column name')
599 | column = get_column(column_name, alias)
| ^^^^^ Expected `AliasedClass[Unknown]`, found `AliasedClass[Unknown] | None`
600 | expr = self.get_filter_expr_from_column(operator, value, column)
|
info: Function defined here
--> src/aiida/storage/psql_dos/orm/querybuilder/main.py:900:5
|
900 | def get_column(colname: str, alias: AliasedClass) -> InstrumentedAttribute:
| ^^^^^^^^^^ ------------------- Parameter declared here
901 | """Return the column for a given projection."""
902 | table_name = get_table_name(alias)
|
info: rule `invalid-argument-type` is enabled by default
Found 2 diagnostics |
Most of this module is already typed, so just made some small adjustments.
I looked into this module as I was testing the new
ty
typechecker from Astral (uv, ruff) and it found an actual bug in an error path in this module, see comment below.