Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/rovr/__main__.py (1)
393-396: Consider adding a log message when libuv is successfully enabled.For debugging and user awareness, it may be helpful to log when the alternative event loop is activated, especially since this is an optional feature. However, this is entirely optional.
💡 Optional: Add debug logging
try: import asyncio if sys.platform == "win32": import winloop as libuv else: import uvloop as libuv asyncio.set_event_loop_policy(libuv.EventLoopPolicy()) + if is_dev: + pprint(" [dim]Using libuv event loop[/]") except (ModuleNotFoundError, ImportError): pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rovr/__main__.py` around lines 393 - 396, Add a debug/info log when an alternative event loop is enabled: after the sys.platform check that imports winloop or uvloop into the name libuv, call the module logger to emit a message like "enabled winloop" or "enabled uvloop" (use the same logger used elsewhere in the module, e.g., getLogger(__name__) or the existing module logger) so it's clear which event loop was activated; place the log immediately after the import in the if/else branches referencing sys.platform, winloop, uvloop and libuv.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/rovr/__main__.py`:
- Around line 393-396: Add a debug/info log when an alternative event loop is
enabled: after the sys.platform check that imports winloop or uvloop into the
name libuv, call the module logger to emit a message like "enabled winloop" or
"enabled uvloop" (use the same logger used elsewhere in the module, e.g.,
getLogger(__name__) or the existing module logger) so it's clear which event
loop was activated; place the log immediately after the import in the if/else
branches referencing sys.platform, winloop, uvloop and libuv.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
pyproject.tomlsrc/rovr/__main__.pysrc/rovr/variables/constants.py
There was a problem hiding this comment.
Pull request overview
Adds an optional “libuv” runtime extra to use a faster event loop implementation (uvloop on Unix-like platforms, winloop on Windows) and wires it into the CLI entrypoint so it’s enabled when installed.
Changes:
- Added
libuvoptional dependency extra (uvloop/winloop) to packaging metadata and lockfile. - Added best-effort runtime initialization of the libuv-backed asyncio event loop policy at startup.
- Updated platform detection for
os_typeto usesys.platformmatching.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| uv.lock | Locks new optional extra dependencies uvloop / winloop under libuv. |
| src/rovr/variables/constants.py | Switches os_type derivation from platform.system() to sys.platform match + explicit mapping. |
| src/rovr/main.py | Attempts to import uvloop/winloop and set the asyncio event loop policy when available. |
| pyproject.toml | Defines the libuv extra for uvloop/winloop with platform markers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/rovr/variables/constants.py
Outdated
| match sys.platform: | ||
| case "win32": | ||
| os_type = "Windows" | ||
| case "darwin": | ||
| os_type = "Darwin" | ||
| case linux if linux.startswith("linux"): | ||
| os_type = "Linux" | ||
| case "android": | ||
| os_type = "Linux" | ||
| case _: | ||
| raise NotImplementedError(f"Unsupported platform: {sys.platform}") |
There was a problem hiding this comment.
constants.py is imported widely (e.g., rovr.variables.constants.os_type). Changing the default branch to raise NotImplementedError introduces a hard import-time failure on any unrecognized sys.platform (e.g., *bsd/cygwin), whereas previously platform.system() would return a string and allow the app to start. Consider falling back to platform.system() (or a safe default like "Linux"/sys.platform) instead of raising at import time, unless this is an intentional breaking change documented elsewhere.
src/rovr/__main__.py
Outdated
| else: | ||
| import uvloop as libuv # ty: ignore[unresolved-import] | ||
| asyncio.set_event_loop_policy(libuv.EventLoopPolicy()) | ||
| except (ModuleNotFoundError, ImportError): |
There was a problem hiding this comment.
ModuleNotFoundError is a subclass of ImportError, so except (ModuleNotFoundError, ImportError) is redundant here. Catching only ImportError is equivalent and simpler.
| except (ModuleNotFoundError, ImportError): | |
| except ImportError: |
|
@coderabbitai do you think this is worth it? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 34826 🌐 Web query: 💡 Result: Yes—Textual is asyncio-based, so it can run on uvloop anywhere uvloop is supported (Unix-like OSes). Textual’s Practical pattern (recommended vs. global policy)Create a uvloop loop and pass it to Textual: import asyncio
import uvloop
from my_app import MyApp
if __name__ == "__main__":
loop = uvloop.new_event_loop()
asyncio.set_event_loop(loop)
MyApp().run(loop=loop)This avoids relying on Platform notes
Sources: [1] Textual
⚖️ Trade-offs AnalysisPotential Benefits
Concerns1. Questionable impact for TUI workloads
The real-world performance improvement may be imperceptible to users. 2. Deprecated implementation pattern Better approach (if you proceed): # In __main__.py, replace the policy setup with:
try:
if sys.platform == "win32":
import winloop as libuv
else:
import uvloop as libuv
loop = libuv.new_event_loop()
asyncio.set_event_loop(loop)
# Then pass loop to Application if it accepts it
except ImportError:
loop = NoneRecommendationWithout benchmarks showing measurable improvement, I'd say this adds complexity for uncertain gains. Consider:
What specific performance issues are you trying to address? That would help determine if this is the right solution. |
|
Will be closing it for now, hopefully I don't delete this branch... |
Summary by CodeRabbit
New Features
Chores