Skip to content

Forward decorated function type to provide_session reusult#20131

Merged
potiuk merged 1 commit intoapache:mainfrom
astronomer:mypy-plugin-provide-session
Dec 8, 2021
Merged

Forward decorated function type to provide_session reusult#20131
potiuk merged 1 commit intoapache:mainfrom
astronomer:mypy-plugin-provide-session

Conversation

@uranusjr
Copy link
Member

@uranusjr uranusjr commented Dec 8, 2021

Now that we have NEW_SESSION, we don’t really need any more special hacks to make provide_session-decorated functions be type-checked. This is enough to type check the following:

from sqlalchemy.orm import Session
from airflow.utils.session import NEW_SESSION, provide_session

@provide_session
def foo(x: int, *, session: Session = NEW_SESSION) -> str:
    return str(x)

foo(x="a")
foo(y=1)

and emit

Run mypy.................................................................Failed
- hook id: mypy
- exit code: 1

No need to rebuild the image: none of the important files changed

airflow/play.py:8: error: Argument "x" to "foo" has incompatible type "str";
expected "int"
    foo(x="a")
          ^
airflow/play.py:9: error: Unexpected keyword argument "y" for "foo"
    foo(y=1)
    ^
Found 2 errors in 1 file (checked 2 source files)

ERROR: The previous step completed with error. Please take a look at output above

For some reason, this does not type check session—I can pass literally anything to it (e.g. session=object()) and Mypy would let it pass. Not sure why, maybe because SQLAlchemy is not well-typed? But this is still very useful.

@potiuk
Copy link
Member

potiuk commented Dec 8, 2021

Interesting, but indeed helpful :)

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Dec 8, 2021
@github-actions
Copy link

github-actions bot commented Dec 8, 2021

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk potiuk merged commit f23769f into apache:main Dec 8, 2021
@uranusjr uranusjr deleted the mypy-plugin-provide-session branch December 8, 2021 11:54
@ashb
Copy link
Member

ashb commented Dec 8, 2021

For some reason, this does not type check session—I can pass literally anything to it (e.g. session=object()) and Mypy would let it pass. Not sure why, maybe because SQLAlchemy is not well-typed?

Correct. It's because our version of SQLA isn't currently typed (1.4 is) and we have ignore_missing_imports I think -- so Session is treated as Any, as I understand it.

@potiuk potiuk added the mypy Fixing MyPy problems after bumpin MyPy to 0.990 label Dec 13, 2021
@kaxil kaxil added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Jan 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-tools changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) full tests needed We need to run full set of tests for this PR to merge mypy Fixing MyPy problems after bumpin MyPy to 0.990

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants