Conversation
WalkthroughAdds CI and pre-commit tooling, expands pyproject development config, updates README publishing docs, exposes DesignerPlugin via Changes
Sequence DiagramsequenceDiagram
participant User
participant DP as DesignerPlugin
participant ZC as Zeroconf / AsyncZeroconf
rect rgb(230, 245, 255)
Note over User,DP: Synchronous enter → register (uses private _zeroconf)
User->>DP: __enter__()
DP->>DP: create _zeroconf
DP->>ZC: register_service()
ZC-->>DP: registered
DP-->>User: self
end
rect rgb(255, 230, 230)
Note over User,DP: Synchronous exit → unregister & close
User->>DP: __exit__()
DP->>ZC: unregister_service()
DP->>ZC: close()
DP->>DP: clear _zeroconf
end
rect rgb(230, 255, 230)
Note over User,DP: Async enter → register (uses private _azeroconf)
User->>DP: __aenter__()
DP->>DP: create _azeroconf (async)
DP->>ZC: register_service() [async]
ZC-->>DP: registered
DP-->>User: self
end
rect rgb(255, 245, 230)
Note over User,DP: Async exit → unregister & close (async)
User->>DP: __aexit__()
DP->>ZC: unregister_service() [async]
DP->>ZC: close() [async]
DP->>DP: clear _azeroconf
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/ci.yml (1)
12-14: Consider testing multiple Python versions.The package supports Python >=3.11 (per
pyproject.toml), but CI only tests 3.11. Consider adding 3.12 and 3.13 to the matrix to ensure compatibility across supported versions.Apply this diff to expand test coverage:
strategy: matrix: - python-version: ["3.11"] + python-version: ["3.11", "3.12", "3.13"]src/designer_plugin/designer_plugin.py (1)
77-77: Replace# type: ignorewith proper type hints.The
__exit__and__aexit__parameters can be properly typed instead of using# type: ignorecomments.Apply this diff to add proper type hints:
-def __exit__(self, exc_type, exc_value, traceback): # type: ignore +def __exit__( + self, + exc_type: type[BaseException] | None, + exc_value: BaseException | None, + traceback: object, +) -> None: if self._zeroconf: self._zeroconf.close() self._zeroconf = None -async def __aexit__(self, exc_type, exc_value, traceback): # type: ignore +async def __aexit__( + self, + exc_type: type[BaseException] | None, + exc_value: BaseException | None, + traceback: object, +) -> None: if self._azeroconf: await self._azeroconf.async_close() self._azeroconf = NoneAlso applies to: 87-87
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.github/workflows/ci.yml(1 hunks).pre-commit-config.yaml(1 hunks)README.md(2 hunks)pyproject.toml(1 hunks)src/designer_plugin/__init__.py(1 hunks)src/designer_plugin/designer_plugin.py(2 hunks)test/__init__.py(1 hunks)test/test_plugin.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/test_plugin.py (1)
src/designer_plugin/designer_plugin.py (2)
DesignerPlugin(9-90)service_info(54-70)
🔇 Additional comments (8)
src/designer_plugin/__init__.py (1)
1-3: LGTM! Explicit public API definition.Adding
__all__clearly defines the module's public interface, which is a best practice for library packages.test/test_plugin.py (1)
1-4: LGTM! Improved import organization.Separating imports and adding explicit JSON handling utilities (
JSONDecodeError,json_dumps) improves clarity and prepares for robust JSON error handling.README.md (1)
13-24: LGTM! Improved documentation clarity.The restructured sections and explicit
d3plugin.jsonexample significantly improve the getting-started experience for plugin developers.pyproject.toml (1)
31-105: LGTM! Comprehensive development tooling configuration.The dev dependencies and tool configurations for ruff, mypy, and pytest are well-structured. The strict mypy settings and appropriate exclusions align with modern Python best practices.
src/designer_plugin/designer_plugin.py (4)
1-7: LGTM! Improved imports with explicit typing.The addition of explicit imports and proper typing support modernizes the codebase and aligns with the new mypy configuration.
12-31: LGTM! Enhanced type safety and lifecycle management.The explicit type hints and introduction of private
_zeroconfand_azeroconfattributes provide better lifecycle control and prevent external interference with the Zeroconf instances.
33-51: LGTM! Type-safe static constructors.The type hints and simplified file handling improve clarity and type safety. The use of
.get()with defaults for optional JSON fields is appropriate.
54-70: LGTM! Properly typed service info property.The explicit
ServiceInforeturn type and standardized string literals improve type safety and consistency.
| async def __aenter__(self) -> "DesignerPlugin": | ||
| self._azeroconf = AsyncZeroconf() | ||
| asyncio.create_task(self._azeroconf.async_register_service(self.service_info)) | ||
| return self |
There was a problem hiding this comment.
Critical: Async service registration may not complete before context entry.
Line 84 creates a background task for service registration but returns immediately without awaiting it. This creates a race condition where the context manager appears "entered" but the service may not be registered yet, potentially causing the plugin to be invisible to Designer.
Apply this diff to await registration completion:
async def __aenter__(self) -> "DesignerPlugin":
self._azeroconf = AsyncZeroconf()
- asyncio.create_task(self._azeroconf.async_register_service(self.service_info))
+ await self._azeroconf.async_register_service(self.service_info)
return self📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def __aenter__(self) -> "DesignerPlugin": | |
| self._azeroconf = AsyncZeroconf() | |
| asyncio.create_task(self._azeroconf.async_register_service(self.service_info)) | |
| return self | |
| async def __aenter__(self) -> "DesignerPlugin": | |
| self._azeroconf = AsyncZeroconf() | |
| await self._azeroconf.async_register_service(self.service_info) | |
| return self |
🤖 Prompt for AI Agents
In src/designer_plugin/designer_plugin.py around lines 82 to 85, the async
context manager starts service registration with asyncio.create_task and returns
immediately, causing a race where the plugin may appear entered before the
Zeroconf service is registered; change this to await
self._azeroconf.async_register_service(self.service_info) (i.e., call and await
the registration coroutine directly), optionally wrap it in try/except to log or
re-raise registration errors and ensure proper cleanup on failure so the context
only returns after registration completes.
| self.name: str = name | ||
| self.port: int = port | ||
| self.hostname: str = hostname or socket.gethostname() | ||
| self.custom_url: str | None = url |
There was a problem hiding this comment.
There's no benefit repeating the type annotations here
There was a problem hiding this comment.
I've removed them and agree they are not necessary.
Originally, I've put it because I like having explicit type hint for all class attributes.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/designer_plugin/designer_plugin.py (1)
82-85: Critical: Async service registration may not complete before context entry.Line 84 creates a background task for service registration but returns immediately without awaiting it. This creates a race condition where the context manager appears "entered" but the service may not be registered yet, potentially causing the plugin to be invisible to Designer.
Apply this diff to await registration completion:
async def __aenter__(self) -> "DesignerPlugin": self._azeroconf = AsyncZeroconf() - asyncio.create_task(self._azeroconf.async_register_service(self.service_info)) + await self._azeroconf.async_register_service(self.service_info) return self
🧹 Nitpick comments (3)
src/designer_plugin/designer_plugin.py (3)
40-52: Consider adding error handling for JSON parsing.The method uses a context manager correctly and has clean type annotations. However, missing required keys (like
"name") will raiseKeyError, and invalid JSON will raiseJSONDecodeError. Consider whether explicit error handling with informative messages would improve the developer experience.Example:
def from_json_file(file_path: str, port: int, hostname: str | None = None) -> "DesignerPlugin": """Convert a JSON file (expected d3plugin.json) to PluginOptions. hostname and port are required.""" - with open(file_path) as f: - options = json_load(f) + try: + with open(file_path) as f: + options = json_load(f) + except (FileNotFoundError, JSONDecodeError) as e: + raise ValueError(f"Failed to load plugin config from {file_path}: {e}") from e + + if "name" not in options: + raise ValueError(f"Missing required 'name' field in {file_path}") + return DesignerPlugin( name=options["name"], port=port,
72-80: LGTM! Proper sync context manager implementation.The synchronous context manager correctly initializes, registers, and cleans up the Zeroconf service. The None check in
__exit__ensures safe cleanup.Optional: The
# type: ignoreon line 77 could be avoided by properly typing the parameters:def __exit__( self, exc_type: type[BaseException] | None, exc_value: BaseException | None, traceback: object ) -> None:
87-90: Proper async cleanup; consider typing the parameters.The async cleanup logic correctly awaits the close operation and guards against None. The
# type: ignoreon line 87 could optionally be removed with proper parameter types (same suggestion as the sync version).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/designer_plugin/designer_plugin.py(3 hunks)
🔇 Additional comments (4)
src/designer_plugin/designer_plugin.py (4)
1-7: LGTM! Clean imports.The imports are well-organized and all are used within the file.
12-31: LGTM! Well-typed constructor with proper lifecycle state.The explicit type annotations using Python 3.10+ union syntax are clean, and the private
_zeroconfand_azeroconfattributes properly support the context manager lifecycle.
33-37: LGTM! Properly typed factory method.The return type annotation is correct and the method provides a clean convenience interface.
54-70: LGTM! Correct ServiceInfo construction.The property correctly returns a
ServiceInfoobject with properly encoded byte properties. The implementation aligns with Zeroconf requirements.
3 participants
Package python plugin
Summary
Before I integrate the d3sdk(d3blobgen), I'd like to add several CI stuff:
Note that I've excluded
/testfrom all above because I don't want to make writing test harder. It should be as easy as possible and lint/typecheck doesn't benefit as much IMO.The client of this package will not require to install any of
mypy,pre-commitorruff.Summary by CodeRabbit
Documentation
Chores
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.