fix: pass black, ruff, and mypy checks#284
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request addresses Python linting and type checking issues to ensure the codebase passes black, ruff check, and mypy validation. The changes add comprehensive type annotations throughout the codebase, fix various linting issues, and update tooling configuration to modern standards.
Changes:
- Added return type annotations (
-> None) and variable type annotations across all modules - Fixed ruff linting issues (C416 dict comprehensions, F821 type checking imports, F841 unused variables, E402 import ordering)
- Added defensive null checks for socket and thread operations to improve type safety
- Migrated configuration from deprecated patterns (ruff config format, Pydantic v1
constr()) to modern equivalents
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Migrated ruff config to [tool.ruff.lint] format (v0.5+), added msgpack to mypy ignore list, configured per-file ignores for E402 |
| server.py | Added comprehensive type annotations for sockets, threads, and queues; defensive null checks; cast() for TOML parsing; resource module import handling |
| rest_bridge.py | Migrated from deprecated constr() to Annotated[str, StringConstraints()] for Pydantic v2 compatibility; added TYPE_CHECKING import for uvicorn |
| nv_sync.py | Simplified dict comprehensions (C416 fix); added explicit bytes() conversion for msgpack |
| logging_utils.py | Updated type aliases for rotation and retention rules; added explicit type annotations |
| events.py | Added return type annotations to callback functions |
| client_simulator.py | Added comprehensive type annotations for ZMQ sockets, threads, and movement strategies; added signal handler types |
| client.py | Added defensive null check in receive loop; configured E402 per-file ignore for conditional imports |
| binary_serializer.py | Added explicit type annotations for result dictionaries |
| adapters.py | Added explicit type annotation for result dictionary |
| tests/integration/test_client.py | Prefixed unused variable with underscore and added noqa comment |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Encode a payload dictionary using MessagePack.""" | ||
|
|
||
| return msgpack.dumps(payload, use_bin_type=True) | ||
| return bytes(msgpack.dumps(payload, use_bin_type=True)) |
There was a problem hiding this comment.
The explicit bytes() conversion is unnecessary here. The msgpack.dumps function already returns bytes when use_bin_type=True. This conversion adds overhead without benefit. Consider removing the explicit bytes() wrapper.
| return bytes(msgpack.dumps(payload, use_bin_type=True)) | |
| return msgpack.dumps(payload, use_bin_type=True) |
| socks = dict(poller.poll(100)) # 100ms timeout | ||
|
|
||
| if self._sub_socket in socks: | ||
| if self._sub_socket in socks and self._sub_socket is not None: |
There was a problem hiding this comment.
The condition self._sub_socket in socks and self._sub_socket is not None is redundant. If self._sub_socket is in socks, it cannot be None. The in operator would have raised an exception if self._sub_socket were None. Consider simplifying to just if self._sub_socket in socks: or reordering to check is not None first if defensive programming is desired.
| if self._sub_socket in socks and self._sub_socket is not None: | |
| if self._sub_socket is not None and self._sub_socket in socks: |
| resource: ModuleType | None | ||
| try: | ||
| import resource | ||
| import resource as _resource | ||
|
|
||
| resource = _resource | ||
| except Exception: | ||
| resource = None # type: ignore[assignment] | ||
| resource = None |
There was a problem hiding this comment.
The resource import pattern here is overly complex. The module-level annotation resource: ModuleType | None followed by conditional assignment creates unnecessary indirection. A simpler approach would be to just import directly with a type ignore comment or use the existing try/except pattern without the intermediate variable. The current pattern makes the code harder to follow without clear benefit.
| message_parts = self.sub_socket.recv_multipart() | ||
| if len(message_parts) >= 2: | ||
| room_id = message_parts[0].decode("utf-8") | ||
| _room_id = message_parts[0].decode("utf-8") # noqa: F841 |
There was a problem hiding this comment.
Variable _room_id is not used.
| _room_id = message_parts[0].decode("utf-8") # noqa: F841 |
Summary
black,ruff check, andmypyChanges
Type Annotations
-> None) to all functions/methodsdict[str, Any],Socket[bytes] | None, etc.)Linting Fixes (ruff)
dict(d)instead of{k: v for k, v in d.items()})TYPE_CHECKINGimport foruvicorntype hintType Safety Improvements (mypy)
device_idtype before processingcast()for TOML version parsingConfiguration Updates
[tool.ruff.lint]format (v0.5+)msgpackto mypy ignore list (missing stubs)constr()toAnnotated[str, StringConstraints()](Pydantic v2)Test Plan
black src/ tests/ --checkpassesruff check src/ tests/passesmypy src/passes (0 errors)styly-netsync-server)