Annotations for psycopg2.ConnectionInfo #7834
Conversation
These annotations come from the documentation here: https://www.psycopg.org/docs/extensions.html#psycopg2.extensions.ConnectionInfo If there was doubt, I referred to the libpq documentation cited by psycopg2's docs. I wasn't completely sure about `dsn_parameters`. Psycopg2's docs list it as an `dict`, and the example suggests it's a `dict[str, str]` at that. From psycopg2's source I found https://github.com/psycopg/psycopg2/blob/1d3a89a0bba621dc1cc9b32db6d241bd2da85ad1/psycopg/conninfo_type.c#L183-L206 which is implemented here: https://github.com/psycopg/psycopg2/blob/1d3a89a0bba621dc1cc9b32db6d241bd2da85ad1/psycopg/utils.c#L251-L279 I'm no expert in CPython's API, but this looks to me like it's building a `dict[str, str]`. Additionally, the libpq docs https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-PQCONNINFO https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-PQCONNDEFAULTS show that the underlying data just consists of strings. Additionally, I'm pretty sure from this chunk of source https://github.com/psycopg/psycopg2/blob/1d3a89a0bba621dc1cc9b32db6d241bd2da85ad1/psycopg/conninfo_type.c#L581-L598 That `ConnectionInfo.__init__` takes one positional-only argument, which must be a `psycopg2.connection`. But I don't think users are intended to be constructing this type, so I've not added that annotation.
DMRobertson
force-pushed
the
dmr/psycopg2/connection-info
branch
from
119e390
to
47658db
Compare
This comment has been minimized.
This comment has been minimized.
|
(Some extra context: I was trying to use these annotations in matrix-org/synapse and found out that |
This comment has been minimized.
This comment has been minimized.
|
Actually, I think I probably ought to annotate the corresponding attributes on |
This comment has been minimized.
This comment has been minimized.
stubs/psycopg2/psycopg2/_psycopg.pyi
Outdated
| status: int | ||
| transaction_status: int | ||
| used_password: bool | ||
| user: str |
There was a problem hiding this comment.
On this and the other parameters possibly being None: it looks like None is returned only when the corresponding libpq functions return NULL. I haven't done a thorough audit, but from a quick scan of https://github.com/postgres/postgres/blob/master/src/interfaces/libpq/fe-connect.c#L6754 it looks like libpq only returns NULL if you give it a NULL ptr in the first place; in some places they explicitly choose to return an empty string rather than NULL.
Given that, plus the fact that psycopg2's docs never mention None for these types, I think I'd be inclined to leave these types as they are. Does that sound reasonable?
There was a problem hiding this comment.
Sounds good, but could you add a comment to the code pointing out that while technically they can be None, that's highly unlikely in practice?
There was a problem hiding this comment.
Certainly, see ed4d8df
stubs/psycopg2/psycopg2/_psycopg.pyi
Outdated
| readonly: Any | ||
| server_version: Any | ||
| server_version: int |
There was a problem hiding this comment.
Also read-only. This is true for all the other attributes listed here: https://github.com/psycopg/psycopg2/blob/8ef195f2ff187454cc709d7857235676bb4176ee/psycopg/connection_type.c#L1244
There was a problem hiding this comment.
Does that include notices, notifies and cursor_factory? They aren't marked with READONLY in that array of structs.
The psycopg2 docs say that notices and notifies are user-writable since psycopg 2.7. No clues on cursor_factory though.
There was a problem hiding this comment.
Good catch, I do think those are writable.
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
|
Thanks for your thoroughness @JelleZijlstra! |
These annotations come from the documentation here. If there was doubt, I referred to the libpq documentation cited by psycopg2's docs.
I wasn't completely sure about
dsn_parameters. Psycopg2's docs list it as andict, and the example suggests it's adict[str, str]at that. From psycopg2's source I found this, with its implementation here. I'm no expert in CPython's API, but this looks to me like it's building adict[str, str]. Additionally, the libpq docs here and here show that the underlying data just consists of C strings.Additionally, I'm pretty sure from this chunk of source that
ConnectionInfo.__init__takes one positional-only argument, which must be apsycopg2.connection. But I don't think users are intended tobe constructing this type, so I've not added that annotation.