Add type annotations to fdb Python bindings (PEP 561)#13241
Conversation
…l, drop unused imports
xis19
left a comment
There was a problem hiding this comment.
Thanks for the implementation!
| ) | ||
|
|
||
| def set(self, key, value): | ||
| def set(self, key: Any, value: Any) -> None: |
There was a problem hiding this comment.
I would feel bringing a Protocol might be helpful, e.g.
class IFoundationDBKey:
def as_foundationdb_key(self) -> bytes:
pass
and key: Union[IFoundationDBKey, bytes]
Not sure if IFoundationDBKey is good but not a native English speaker, so do not know if there is any better choices.
There was a problem hiding this comment.
I've introduced IFoundationDBKey and IFoundationDBValue as @runtime_checkable Protocols so the type reflects the actual duck-typing contract the code uses. All key/value parameters now use KeyType = Union[bytes, IFoundationDBKey] and ValueType = Union[bytes, IFoundationDBValue].
|
|
||
| def __init__(self, fpointer): | ||
| def __init__(self, fpointer: Any) -> None: | ||
| # print("Creating future 0x%x" % fpointer) |
There was a problem hiding this comment.
Maybe help removing those debugging prints? Or use logging.Logger?
There was a problem hiding this comment.
the commented-out debug prints in Future.init and del have been removed.
| def on_ready(self, callback): | ||
| def cb_and_delref(ignore): | ||
| def on_ready(self, callback: Callable[[Future], None]) -> None: | ||
| def cb_and_delref(ignore: object) -> None: |
There was a problem hiding this comment.
Oh, maybe just _ignore: Any?
There was a problem hiding this comment.
Changed _ignore: Any → _ignore: object in both cb_and_delref and wait_for_any's closure
| for i, f in enumerate(futures): | ||
|
|
||
| def cb(ignore, i=i): | ||
| def cb(ignore: object, i: int = i) -> None: |
There was a problem hiding this comment.
maybe rename the arg i to something else? should be better than shadowing variable
| self.on_ready(lambda f: self.call_soon_threadsafe(fn, f)) | ||
|
|
||
| def remove_done_callback(self, fn): | ||
| def remove_done_callback(self, fn: Callable[[Future], None]) -> None: |
There was a problem hiding this comment.
Yes, it should be and I marked it @abc.abstractmethod to match wait(). It always raises NotImplementedError and none of the concrete subclasses (FutureVoid, FutureInt64, etc.) implement it, so making it abstract is the right signal."
|
|
||
| class KeySelector(object): | ||
| def __init__(self, key, or_equal, offset): | ||
| key: bytes |
There was a problem hiding this comment.
I am confused about this...
There was a problem hiding this comment.
That was a class-level instance variable annotation (PEP 526) that I had added above init, which the diff view made look like it was inside the method body. It was actually redundant as type checkers already infer self.key: bytes from the init parameter annotation, so I've removed the class-level declarations and kept only the init parameter annotations. Sorry for the confusion in the diff!
| import fdb | ||
|
|
||
| # Type alias for values that can be packed into a tuple | ||
| TupleElement = Union[ |
There was a problem hiding this comment.
TupleElement: TypeAlias = Union[ ...
There was a problem hiding this comment.
Used TupleElement: TypeAlias = Union[...] directly. Since TypeAlias was added in Python 3.10 and we support 3.8+, I added a sys.version_info guard with a fallback to typing_extensions, and added typing_extensions>=4.0 as a conditional dependency in pyproject.toml
| _UNSET_TR_VERSION = 10 * int2byte(0xFF) | ||
| _STRUCT_FORMAT_STRING = ">" + str(_TR_VERSION_LEN) + "sH" | ||
|
|
||
| tr_version: Optional[bytes] |
There was a problem hiding this comment.
The class-level constants (LENGTH, _TR_VERSION_LEN, _MAX_USER_VERSION, etc.) are now annotated with ClassVar[int]/ClassVar[bytes]/ClassVar[str] as appropriate. The tr_version and user_version fields are instance variables (set in init) so they do not use ClassVar, They're declared as plain instance variable annotations in the class body.
|
lgtm |
|
Hi @xis19 — thanks for the review! Would you be able to submit a formal approval? |
Problem
Addresses issue #11331. The
fdbPython bindings ship without type annotations and without apy.typedmarker. Downstream users get no help from mypy, pyright, or IDEs when calling the public API, and even if annotations existed they would be ignored by PEP 561-compliant type checkers (which require an explicitpy.typedfile to trust inline annotations from a third-party package).Solution
fdb/py.typedmarker file and wire it into the sdist/wheel via[tool.setuptools.package-data]inpyproject.toml(PEP 561).fdb/impl.py,fdb/tuple.py,fdb/subspace_impl.py, andfdb/locality.py.TupleElementtype alias infdb/tuple.pycovering the values the tuple layer can pack (None | bytes | str | int | float | bool | uuid.UUID | SingleFloat | Versionstamp | nested tuple/list), and use it inpack,pack_with_versionstamp,unpack,range,has_incomplete_versionstamp,compare, and acrossSubspace.from __future__ import annotationsso all annotations are strings at runtime — zero runtime cost, no import-order issues.Where
Anyis intentionally retainedA few annotations remain
Anyon purpose:Future.wait()/Future.result()return type —Futureis a generic base class; concrete subclasses (FutureInt64,FutureString,FutureKeyValueArray, …) already return narrower types.keyToBytes(k)/valueToBytes(v)parameters — duck-typed via theas_foundationdb_key/as_foundationdb_valueprotocol. Tightening these would require introducing atyping.Protocol; happy to do that in a follow-up.tpointer/fpointer/dpointerconstructor params onTransactionRead,Transaction,Future,Database— the codebase passes a mix ofint,Optional[int](fromc_void_p().value), and rawctypes.c_void_pto these. A single accurate type would require touching call sites, which expands scope.*args: Any, **kwargs: AnyinTransaction.get_range_startswith— pure forwarding toget_range.Testing
Ran mypy against both
mainand this branch using: