[synth] Adding Sequential Fourm - #535
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughA new adaptation module registers support for the Sequential Fourm device with a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Merge Risk: ⚪ Minimal · up to This adds beta Sequential Fourm support, including packaging and documentation, with automated coverage for its key MIDI, bank, and import workflows. No concrete merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 1📝 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.
🧹 Nitpick comments (3)
adaptations/test_Sequential_Fourm.py (1)
17-22: Round-trip check is solid; the length asserts are largely redundant.The byte-for-byte reconstruction at line 22 already implies the length asserts at lines 20–21. Keeping them is fine as documentation of expected payload sizes, but a brief comment explaining that 4102 is the unpacked Fourm program payload (and 4688 is its 7-to-8 nibblized form) would help future readers diagnose a failure here without having to re-derive the math. No functional concern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@adaptations/test_Sequential_Fourm.py` around lines 17 - 22, Add a short inline comment above the length asserts explaining what the magic numbers represent: 4102 is the expected size of the unpacked Fourm program payload and 4688 is the corresponding 7-to-8 nibblized (escaped) size; reference the variables and functions involved (unpacked, repacked, message and sequential.GenericSequential.unescapeSysex / escapeSysex) so future readers can immediately understand the origin of those numbers without re-deriving the math.adaptations/Sequential_Fourm.py (2)
28-41: Return-type annotations don't match the generator bodies.Both
programsandedit_buffersuseyield, so they are generator functions, not functions returningList[...]. The annotations are misleading (and would be wrong if any caller treats them aslist).♻️ Suggested annotation fix
-from typing import List +from typing import Iterator @@ - def programs(data: testing.TestData) -> List[testing.ProgramTestData]: + def programs(data: testing.TestData) -> Iterator[testing.ProgramTestData]: @@ - def edit_buffers(data: testing.TestData) -> List[testing.ProgramTestData]: + def edit_buffers(data: testing.TestData) -> Iterator[testing.ProgramTestData]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@adaptations/Sequential_Fourm.py` around lines 28 - 41, Both programs and edit_buffers are generator functions (they use yield) but are annotated as returning List[testing.ProgramTestData]; change their return annotations to a generator/iterator type such as Iterator[testing.ProgramTestData] or Generator[testing.ProgramTestData, None, None] and add the corresponding import from typing (Iterator or Generator) at the top; update the def signatures for programs(...) and edit_buffers(...) to use the chosen annotation (e.g., def programs(...) -> Iterator[testing.ProgramTestData]:) so the type matches the actual generator behavior.
36-41: Ruff F821 flag is valid for this bare name — usethis_module.convertToEditBufferto clarify runtime injection.
convertToEditBufferis dynamically injected bysequential.GenericSequential.install(this_module)at lines 17–24, which registers it on the module viasetattr(). At runtime this works correctly, but Ruff's static analysis cannot see the dynamic injection and flags the bare name as undefined (F821). Sincethis_moduleis already available at line 13, qualifying the reference silences Ruff and improves clarity:Recommended clarity fix
def edit_buffers(data: testing.TestData) -> List[testing.ProgramTestData]: yield testing.ProgramTestData( - message=convertToEditBuffer(0, data.all_messages[0]), + message=this_module.convertToEditBuffer(0, data.all_messages[0]), name="A New Legend", rename_name="Fourm Rename Test", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@adaptations/Sequential_Fourm.py` around lines 36 - 41, The bare name convertToEditBuffer in the generator function edit_buffers triggers Ruff F821; qualify the call via the module object used for dynamic injection (this_module) so static analysis sees it. Update the call in edit_buffers to use this_module.convertToEditBuffer(...) (keeping the same arguments and return shape), referencing the existing this_module symbol that was set up by sequential.GenericSequential.install so the runtime behavior is unchanged but Ruff is satisfied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@adaptations/Sequential_Fourm.py`:
- Around line 28-41: Both programs and edit_buffers are generator functions
(they use yield) but are annotated as returning List[testing.ProgramTestData];
change their return annotations to a generator/iterator type such as
Iterator[testing.ProgramTestData] or Generator[testing.ProgramTestData, None,
None] and add the corresponding import from typing (Iterator or Generator) at
the top; update the def signatures for programs(...) and edit_buffers(...) to
use the chosen annotation (e.g., def programs(...) ->
Iterator[testing.ProgramTestData]:) so the type matches the actual generator
behavior.
- Around line 36-41: The bare name convertToEditBuffer in the generator function
edit_buffers triggers Ruff F821; qualify the call via the module object used for
dynamic injection (this_module) so static analysis sees it. Update the call in
edit_buffers to use this_module.convertToEditBuffer(...) (keeping the same
arguments and return shape), referencing the existing this_module symbol that
was set up by sequential.GenericSequential.install so the runtime behavior is
unchanged but Ruff is satisfied.
In `@adaptations/test_Sequential_Fourm.py`:
- Around line 17-22: Add a short inline comment above the length asserts
explaining what the magic numbers represent: 4102 is the expected size of the
unpacked Fourm program payload and 4688 is the corresponding 7-to-8 nibblized
(escaped) size; reference the variables and functions involved (unpacked,
repacked, message and sequential.GenericSequential.unescapeSysex / escapeSysex)
so future readers can immediately understand the origin of those numbers without
re-deriving the math.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3639c8a5-4191-4d1a-bd1c-07645751555d
📒 Files selected for processing (3)
adaptations/Sequential_Fourm.pyadaptations/testData/Sequential_Fourm/a_new_legend.syxadaptations/test_Sequential_Fourm.py
|
Release preparation pushed in 4b9a0f4, after merging current master:
Python 3.12: 35 passed, 14 skipped. Shipping registration and git diff --check verified. No hardware testing performed. Code work is ready for merge consideration once fresh CI and review pass. |
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests