More backporting pyrefly + vacuum - #203
Conversation
pyrefly setup for circuit drivers and ignore for a code path that wont happen since we would never have an app with no routes
The .camel_case_model import was placed after the is_circuit_python_driver-gated .common/.driver_routes block, so when that flag is true it rendered out of alphabetical order (ruff-src flagged it as a fixable isort violation in the circuit-backend E2E lint job). Also had an extra blank line before API_DESCRIPTION (ruff-format flagged it). Moved the import above the conditional block so it always sorts correctly regardless of is_circuit_python_driver, and removed the stray blank line. Verified against a real Jinja2 render (trim_blocks/lstrip_blocks/ keep_trailing_newline=True, matching copier's defaults) with both ruff check --select I and ruff format run against the project's actual .config/ruff.toml.
is_circuit_python_driver was already a template variable, so gate the resources-plural and no-request-body exemptions directly in vacuum-ignore.yaml.jinja instead of a post-generation copier _task - matches weight-sensor-driver's own local vacuum-ignore.yaml (both exemptions, same justifications), just applied once at the template level since every circuit-python driver hits the same route shapes. resources-plural: a generated driver only ever manages one physical device connection, so /api/driver/* is intentionally singular, not a REST collection. no-request-body: DELETE /device-connection removes a device by mDNS service name with no resource ID in the path, so the body is unavoidable. Also register the "driver" tag in OPENAPI_TAGS, next to debug/mDNS under the same has_circuit_python_backend_template_been_instantiated gate - circuit-backend's driver_routes.py already tags routes with tags=["driver"], but nothing had ever registered that tag, so vacuum's operation-tag-defined fired on every driver route.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe template adds CircuitPython-specific Pyrefly and Vacuum configuration, adds a backend OpenAPI ChangesCircuitPython template support
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 1
🤖 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 `@copier.yml`:
- Around line 394-403: Update the Pyrefly configuration command in the
is_circuit_python_driver block to match the search-path entry explicitly rather
than checking firmware/src anywhere in the file. After the sed update, enforce
that the first ^search-path = entry contains backend_api/firmware/src and fail
the command when the entry is missing or unchanged; preserve the existing
not-found handling for .config/pyrefly.toml.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2c5e8df6-b220-42ec-b744-dcfb0afecb8d
📒 Files selected for processing (4)
copier.ymltemplate/.config/vacuum-ignore.yaml.jinjatemplate/{% if has_backend %}backend{% endif %}/src/backend_api/app_def.py.jinjatemplate/{% if has_backend %}backend{% endif %}/src/backend_api/fast_api_exception_handlers.py
| - "$.paths['/api/driver/device-info-from-serial-port']" | ||
| - "$.paths['/api/driver/device-connection']" | ||
| - "$.paths['/api/driver/send-raw-command']" | ||
| - "$.paths['/api/driver/measurement-types']" |
There was a problem hiding this comment.
NIT: isn't types already plural?
There was a problem hiding this comment.
Issue is not with measurement-types its with driver. Resources plural is ensuring that resources are in fact plural. In this case switching those all our to be drivers felt off specially since some of those debug commands are not using sensor_ids. Wonder if the better answer is we should actually drop driver. And it should become devices and some of the debug ones maybe become something debug specific of /api/devices/debug/ and we just allow list all those. Idk but feels like a future me problem to tweak this api.
There was a problem hiding this comment.
Ah.
I think I was thinking of "driver" originally as more of a "category", because there was were other routes having to do with "mDNS". Yeah, definitely due for some refactoring, and definitely not in this PR
| - "$.paths['/api/shutdown'].get.responses" | ||
|
|
||
| {% endraw %}{% if backend_uses_graphql %}{% raw %}# /api/graphql's 200 response body is arbitrary JSON shaped by the caller's GraphQL query/GraphiQL page; | ||
| {% endraw %}{% if is_circuit_python_driver %}{% raw %}# A generated driver only ever manages a single physical device connection at a time, so its |
There was a problem hiding this comment.
If the route is id/attribute, then that would be a little common singular pattern a lot right, not just for drivers? We may need a refactor later to make vacuum better suited
2 participants
Why is this change necessary?
pyrefly need the same setup that we have for pyright to add the configuration for firmware if its a circuit_python_driver. It also needs some ignores for vacuum in the circuit_python_driver case. We also had a test in a downstream repo that really should never be needed since we will always have an app with routes. So instead of carrying that test up here we just added an ignores.
How does this change address the issue?
Backports the changes
What side effects does this change have?
N/A
How is this change tested?
CI here and tested downstream
Summary by CodeRabbit
New Features
Bug Fixes
Documentation