PYTHON-3867 add types to topology.py #1346
Conversation
sleepyStick
requested review from
NoahStapp and
ShaneHarvey
and removed request for
a team
pymongo/mongo_client.py
Outdated
| @@ -1096,7 +1096,7 @@ def address(self) -> Optional[Tuple[str, int]]: | |||
| return self._server_property("address") | |||
|
|
|||
| @property | |||
| def primary(self) -> Optional[Tuple[str, int]]: | |||
| def primary(self) -> Optional[_Address]: | |||
There was a problem hiding this comment.
Hmm this is a good catch but I'm not sure yet if it's correct. There's actually a subtle difference here between replica sets vs direct connections. In general, _Address can be have port=None because we support connecting directly to a single node via a unix socket path (which don't have ports). Here though we're returning addresses to replica set members which always have a port (I think?).
We should test or find out if unix paths can be used as replica set host names. This wouldn't be practical since it would require the replica set and clients to all live on the same machine but if it's possible I suppose we should make this change.
There was a problem hiding this comment.
If you want to ignore this rabbit hole I suggest undoing the _Address changes in this PR and opening a new jira ticket.
There was a problem hiding this comment.
hm I see, I open a new jira ticket for this then.
test/test_read_preferences.py
Outdated
| @@ -265,7 +265,7 @@ def test_nearest(self): | |||
|
|
|||
| not_used = data_members.difference(used) | |||
| latencies = ", ".join( | |||
| "%s: %dms" % (server.description.address, server.description.round_trip_time) | |||
| "%s: %dms" % (server.description.address, server.description.round_trip_time) # type: ignore[str-format] | |||
There was a problem hiding this comment.
Is it possible to fix the format string to remove the type ignore?
There was a problem hiding this comment.
I believe the error is that round_trip_time is an Optional[float], my first instinct was to add an assert server.description.round_trip_time is not None statement but that doesn't work well with the list comprehension...alternatively, I could cast it? What are your thoughts?
There was a problem hiding this comment.
Can we use %s instead of %d?
pymongo/topology.py
Outdated
| self._max_cluster_time = None | ||
| self._servers: Dict[_Address, Server] = {} | ||
| self._pid: Optional[int] = None | ||
| self._max_cluster_time: Optional[Mapping[str, Any]] = None |
There was a problem hiding this comment.
Can we add a type alias for cluster time called ClusterTime?
pymongo/topology.py
Outdated
| self._session_pool = _ServerSessionPool() | ||
|
|
||
| if self._publish_server or self._publish_tp: | ||
| weak: weakref.ReferenceType[Optional[queue.Queue]] |
There was a problem hiding this comment.
Should be ReferenceType[queue.Queue] (ditch the optional).
There was a problem hiding this comment.
I see now, fixed!
pymongo/topology.py
Outdated
| @@ -430,21 +475,22 @@ def _get_replica_set_members(self, selector): | |||
| ): | |||
| return set() | |||
|
|
|||
| return {sd.address for sd in selector(self._new_selection())} | |||
| # mypy doesn't recognize classes that implement a __getitem__ as something that is iterable | |||
| return {sd.address for sd in selector(self._new_selection())} # type: ignore[attr-defined] | |||
There was a problem hiding this comment.
Does calling iter() explicitly fix this: iter(selector(...))?
pymongo/mongo_client.py
Outdated
| @@ -1906,7 +1906,7 @@ def _send_cluster_time( | |||
| session_time = session.cluster_time if session else None | |||
| if topology_time and session_time: | |||
| if topology_time["clusterTime"] > session_time["clusterTime"]: | |||
| cluster_time = topology_time | |||
| cluster_time: Optional[Mapping[str, Any]] = topology_time | |||
There was a problem hiding this comment.
Let's use ClusterTime here too.
pymongo/topology.py
Outdated
| """Return set of arbiter addresses.""" | ||
| return self._get_replica_set_members(arbiter_server_selector) | ||
|
|
||
| def max_cluster_time(self): | ||
| def max_cluster_time(self) -> Optional[Mapping[str, Any]]: |
There was a problem hiding this comment.
Let's use ClusterTime here and anywhere else we use cluster_time or $clusterTime.
pymongo/topology.py
Outdated
|
|
||
| def process_events_queue(queue_ref): | ||
|
|
||
| def process_events_queue(queue_ref: weakref.ReferenceType[Optional[queue.Queue]]) -> bool: |
There was a problem hiding this comment.
Ditch the optional here as well.
There was a problem hiding this comment.
right, sorry for missing it, thanks for catching that!
pymongo/typings.py
Outdated
| @@ -34,6 +34,7 @@ | |||
| _Address = Tuple[str, Optional[int]] | |||
| _CollationIn = Union[Mapping[str, Any], "Collation"] | |||
| _Pipeline = Sequence[Mapping[str, Any]] | |||
| ClusterTime = Optional[Mapping[str, Any]] | |||
There was a problem hiding this comment.
Personally I think we should nix the Optional here.
There was a problem hiding this comment.
I don't have a huge preference either way, so its changed :)
https://jira.mongodb.org/browse/PYTHON-3867