feat: hot reloading - #88
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds the exported ChangesOxapy hot-reload server
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Application
participant Oxapy
participant Watchdog
participant Worker
Application->>Oxapy: run(reload=True)
Oxapy->>Worker: start with OXAPY_WORKER=1
Oxapy->>Watchdog: monitor configured files
Watchdog->>Oxapy: report file change
Oxapy->>Worker: terminate and restart
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib.rs (1)
439-481: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winShared
runningflag is not connected to Ctrl+C shutdown.
ShutDownSignal::new()creates its ownArc<AtomicBool>, so the ctrlc handler never flipsHttpServer::running.process_requestsexits on the shutdown channel, but the accept loop inspawn_connection_handlerkeeps accepting becauseself.runningstaystrue. Thread the server’sArc<AtomicBool>intoShutDownSignalor stop the shared flag from the handler.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib.rs` around lines 439 - 481, The server shutdown state is split between HttpServer::running and the flag owned by ShutDownSignal, so Ctrl+C does not stop the accept loop. Update setup_serve and the ShutDownSignal construction to share HttpServer::running, or otherwise have the shutdown handler set that existing flag, ensuring spawn_connection_handler observes false and stops accepting connections.
🧹 Nitpick comments (1)
Cargo.toml (1)
18-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin
pyo3-stub-gento a crates.io release.
A compatible 0.22.x release already exists for PyO3 0.28, sobranch = "main"is an unnecessary moving target. If you still need git, pin an explicit commit instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Cargo.toml` around lines 18 - 26, Update the pyo3-stub-gen dependency in the [dependencies] section to use the compatible crates.io 0.22.x release instead of tracking the main branch; preserve its existing default-features and feature settings, and only use a pinned git commit if a registry release cannot be used.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@oxapy/__init__.py`:
- Line 18: Add an inline Ruff suppression for F403 to the wildcard import in
__init__.py, or replace it with explicit re-exports from oxapy. Preserve the
package’s existing public exports and follow the file’s established inline
suppression style.
- Around line 82-150: Update the non-zero exit branch in _run_supervisor so it
logs the worker_process.returncode before waiting for reload_requested. Preserve
the existing restart behavior and wait semantics, while clearly indicating that
the worker exited and the supervisor is waiting for a file change.
- Around line 334-347: Update the exception raises and Response return in the
file-serving flow to use the file’s established `# ty:ignore` convention instead
of `# type:ignore`, matching the existing annotations and the migration applied
near line 219. Preserve any necessary diagnostic code while making all three
affected annotations consistent.
- Line 52: Rename the set_watch_dir parameter dir to a non-shadowing name,
update all references within the method, and apply the same parameter rename in
the corresponding set_watch_dir declaration in the .pyi stub to keep the APIs
synchronized.
In `@tests/app.py`:
- Line 9: Update the main function signature in tests/app.py to include the None
return type annotation, making it def main() -> None while preserving its
existing implementation.
- Line 16: Guard the main() call with an if __name__ == "__main__" entrypoint
check so importing tests.app does not start the server; preserve direct script
execution behavior.
- Around line 4-6: Update the greet handler and health route in tests/app.py to
use explicit typed request handlers: make greet accept request as its first
parameter and annotate it with Request, and replace the health lambda with a
named health(request: Request) -> str function. Preserve both routes’ existing
response behavior.
---
Outside diff comments:
In `@src/lib.rs`:
- Around line 439-481: The server shutdown state is split between
HttpServer::running and the flag owned by ShutDownSignal, so Ctrl+C does not
stop the accept loop. Update setup_serve and the ShutDownSignal construction to
share HttpServer::running, or otherwise have the shutdown handler set that
existing flag, ensuring spawn_connection_handler observes false and stops
accepting connections.
---
Nitpick comments:
In `@Cargo.toml`:
- Around line 18-26: Update the pyo3-stub-gen dependency in the [dependencies]
section to use the compatible crates.io 0.22.x release instead of tracking the
main branch; preserve its existing default-features and feature settings, and
only use a pinned git commit if a registry release cannot be used.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88685447-7f62-443c-bd2f-1bdbc9e002a9
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.helix/languages.tomlCargo.tomloxapy/__init__.pyoxapy/__init__.pyipyproject.tomlsrc/lib.rstests/app.pytests/conftest.py
| @get("/greet/{name}") | ||
| def greet(_r, name: str): | ||
| return f"Hello, {name}!" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- tests/app.py ---\n'
cat -n tests/app.py | sed -n '1,120p'
printf '\n--- search for request handler guidance/usages ---\n'
rg -n "def .*request|`@get`\\(|health lambda|request: Request|from .*Request" tests -S || trueRepository: j03-dev/oxapy
Length of output: 242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n tests/app.py | sed -n '1,120p'Repository: j03-dev/oxapy
Length of output: 589
Use request and type-annotate the handlers. In tests/app.py:4-12, both routes should take request as the first argument, and the health lambda should be a named health(request: Request) -> str handler so both signatures are explicit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/app.py` around lines 4 - 6, Update the greet handler and health route
in tests/app.py to use explicit typed request handlers: make greet accept
request as its first parameter and annotate it with Request, and replace the
health lambda with a named health(request: Request) -> str function. Preserve
both routes’ existing response behavior.
Source: Coding guidelines
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
1 participant
Summary by CodeRabbit
OxapyPython server with optional automatic hot-reloading for development.Oxapy.pylsp).