neo4j / neo4j-python-driver

Neo4j Bolt driver for Python
https://neo4j.com/docs/api/python-driver/current/
Other
883 stars 187 forks source link

Pylance Typing checking error for AsyncTransaction #1015

Closed eric-nguyen-cs closed 5 months ago

eric-nguyen-cs commented 6 months ago

Bug Report

Screenshot 2024-01-31 at 11 52 15

I am using the AsyncTransaction context manager

 async with txn_manager as _txn:
    ...

However, the Pylance VSCode extension (powered by Pyright) in Basic Type checking mode is giving a type error: Object of type "AsyncTransaction" cannot be used with "with" because it does not implement __aenter__ Object of type "AsyncTransaction" cannot be used with "with" because it does not implement __aexit__

I believe that it is because AsyncTransaction's __aenter__ and __aexit__ are wrapped with functools.wraps(AsyncTransactionBase._enter) and functools.wraps(AsyncTransactionBase._exit) respectively

My Environment

Python Version: 3.11 Driver Version: 5.14 Operating System: macOS

robsdedude commented 6 months ago

Hi and thanks for reaching out.

I'm thinking this might rather be an issue with the chosen type checker (Pylance, in your case).

Reasons I'm thinking this:

eric-nguyen-cs commented 5 months ago

Thank you for your quick response

I agree what you said, the bug does seem to come from pyright and not the neo4j code: I ran the code against other type checkers (pyre-check and pytype) and it seems that only pyright raised an issue here.

However, I am wondering if the AsyncTransaction methods should be wrapped with the AsyncTransactionBase ones : the functools.wraps function's documentation seems to indicate that functools.wraps is mostly used for decorated functions. I'm not sure what the value of adding this decorator here: the docstrings could easily be moved to the AsyncTransaction methods, as the AsyncTransactionBase class is not exposed to the library consumers.

robsdedude commented 5 months ago

You are right in that wraps does not seem to be apt here. While I could use the optional arguments to wraps to make it only copy the docs string (and maybe annotations), there's no good reason for this complication. So I cleaned up the code in the PR linked above. Thanks again for reaching out.

eric-nguyen-cs commented 5 months ago

And thank you for quickly responding and solving the issue!