Skip to content
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

typing: use mypy to verify runtime behaviour matches type stubs #338

Closed
AllSeeingEyeTolledEweSew opened this issue Jun 7, 2022 · 4 comments
Assignees

Comments

@AllSeeingEyeTolledEweSew

I'm using apsw from pypi (thanks for that!)

I found some issues with the type stubs.

Some valid code that fails type-checking:

import apsw
conn = apsw.Connection(":memory:")
cur = conn.cursor()
cur.execute("CREATE TABLE t (x INT PRIMARY KEY)")
cur.execute("SELECT * FROM t")
for row in cur:
    pass
row = cur.execute("SELECT * FROM t").fetchone()
$ pip freeze | grep -E '(mypy|apsw)=='
apsw==3.38.5.post1
mypy==0.961
$ mypy test.py
test.py:6: error: "Cursor" has no attribute "__iter__" (not iterable)
test.py:8: error: "Iterator[Any]" has no attribute "fetchone"
Found 2 errors in 1 file (checked 1 source file)
  • Cursor.execute() returns Iterator
    • It should return Cursor. (Actually it should return typing.Self, but that's new in 3.11 beta, and there isn't a nice way to declare this in a backwards-compatible way yet)
  • Cursor doesn't expose __iter__()/__next__() in type stubs.
    • The iterator protocol methods should be exposed in the type stubs, as well as any other methods that are part of the public API.
    • Cursor.__iter__() should return Cursor (or Self)
    • Cursor.__next__() should return Any, or perhaps a narrower row type (IMO the row type should be narrower than Any, but I'll file a separate issue about this)
@rogerbinns rogerbinns self-assigned this Jun 7, 2022
@rogerbinns
Copy link
Owner

I did use stubtest, but that turns out to check stubs exists and are syntactically valid. The primary bug here is that I should also use mypy to verify that runtime behaviour matches the types. Changing title to match.

@rogerbinns rogerbinns changed the title typing: Cursor is not iterable, and Cursor.execute() doesn't return Cursor typing: use mypy to verify runtime behaviour matches type stubs Jun 7, 2022
rogerbinns added a commit that referenced this issue Jun 7, 2022
Although cursor is an iterator.  Refs #338
@AllSeeingEyeTolledEweSew
Copy link
Author

FYI: It's probably "fine" to release changes to type stubs that break the type-checking of existing users, in a non-breaking update of apsw. This is just the state of type checking in python. Every release of mypy seems to include some breaking changes in their type stubs, so I always assume any update of any other typed package might also break.

@rogerbinns
Copy link
Owner

So far you've found bugs in the typing information, so I have no problem fixing them, just like any other bug. I am going to try to run the example code (exercises every api) under mypy --strict

I was a bit surprised at having to expose the __iter__/__next__ methods since that machinery is not normally exposed (for example doc doesn't mention them). The type stubs for sqlite3 do so that is the right thing to do. I'll have to make sure it stays out of the doc though - currently the stubs are generated from the docstrings.

rogerbinns added a commit that referenced this issue Jun 12, 2022
rogerbinns added a commit that referenced this issue Jul 5, 2022
rogerbinns added a commit that referenced this issue Jul 5, 2022
Refs #338

As of this commit, the sample code in the issue works (open db, do a
query).  The example code has more issues to resolve like also accepting
zeroblobs and the Shell.
@rogerbinns
Copy link
Owner

Closing because this is 99% done

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

No branches or pull requests

2 participants