fix: target the symmetric-extrusion property the running FreeCAD has (Pad, Revolution, Groove) - #100
Open
Merlz wants to merge 1 commit into
Open
fix: target the symmetric-extrusion property the running FreeCAD has (Pad, Revolution, Groove)#100Merlz wants to merge 1 commit into
Merlz wants to merge 1 commit into
Conversation
PartDesign::Pad has no `Symmetric` property on any currently supported FreeCAD
release, so `pad_sketch(symmetric=...)` raised AttributeError. FreeCAD exposes
`Midplane`, and 1.1+ supersedes that with `SideType` while emitting a
deprecation warning if `Midplane` is set:
The 'Midplane' property being set for the extrusion of Sketch is deprecated
and has been replaced by the 'SideType' property in FeatureExtrude.
The generated code now probes for the property instead of assuming one, so a
single build works across versions and stays quiet on 1.1+:
SideType (1.1+) -> Midplane (legacy) -> Symmetric (terminal fallback)
`revolution_sketch` and `groove_sketch` were broken the same way. They have no
`SideType`, so they use the `Midplane` chain and are unchanged otherwise. Those
two sites were not covered by the original report.
Verified against FreeCAD 1.1.3: a symmetric pad of length 6 now yields
SideType="Symmetric" and spans z=-3..+3 on a valid solid.
Fixes spkane#94
Contributor
📝 WalkthroughWalkthroughUpdated generated FreeCAD code to select compatible symmetry properties for pads, revolutions, and grooves. Added tests covering property preference, fallbacks, single binding of the symmetric value, and generated-code compilation. ChangesSymmetric property compatibility
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/unit/test_tools_partdesign.py`:
- Around line 1140-1185: Rename the listed non-test helpers and fixtures to
camelCase—mockMcp, mockBridge, registerTools, toolDecorator, getBridge, and
_generated—and add type annotations for every parameter and return value in
those functions. Update all references consistently while preserving the
existing fixture setup and generated-code validation behavior.
- Around line 1196-1201: Strengthen the relevant assertions in the test covering
pad symmetry compatibility: verify every fallback branch assigns each supported
object’s Symmetric property from _symmetric, and replace the current presence
check with code.count("_symmetric =") == 1 to enforce a single binding. Preserve
the existing SideType-before-Midplane ordering assertion and apply the same
checks to the additional branch covered around lines 1215–1230.
🪄 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: 03334d80-b7ba-4505-9d4e-f3bc7f2dcb9c
📒 Files selected for processing (2)
src/freecad_mcp/tools/partdesign.pytests/unit/test_tools_partdesign.py
Comment on lines
+1140
to
+1185
| @pytest.fixture | ||
| def mock_mcp(self): | ||
| mcp = MagicMock() | ||
| mcp._registered_tools = {} | ||
|
|
||
| def tool_decorator(): | ||
| def wrapper(func): | ||
| mcp._registered_tools[func.__name__] = func | ||
| return func | ||
|
|
||
| return wrapper | ||
|
|
||
| mcp.tool = tool_decorator | ||
| return mcp | ||
|
|
||
| @pytest.fixture | ||
| def mock_bridge(self): | ||
| bridge = AsyncMock() | ||
| bridge.execute_python = AsyncMock( | ||
| return_value=ExecutionResult( | ||
| success=True, | ||
| result={"success": True, "name": "F", "label": "F", "type_id": "T"}, | ||
| stdout="", | ||
| stderr="", | ||
| error_traceback=None, | ||
| execution_time_ms=1.0, | ||
| ) | ||
| ) | ||
| return bridge | ||
|
|
||
| @pytest.fixture | ||
| def register_tools(self, mock_mcp, mock_bridge): | ||
| from freecad_mcp.tools.partdesign import register_partdesign_tools | ||
|
|
||
| async def get_bridge(): | ||
| return mock_bridge | ||
|
|
||
| register_partdesign_tools(mock_mcp, get_bridge) | ||
| return mock_mcp._registered_tools | ||
|
|
||
| @staticmethod | ||
| def _generated(mock_bridge): | ||
| code = mock_bridge.execute_python.call_args[0][0] | ||
| # Generated code is exec'd inside FreeCAD, so it must at least parse. | ||
| compile(code, "<generated>", "exec") | ||
| return code |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Apply the required helper naming and annotations.
Rename non-test helpers/fixtures to camelCase and add parameter/return annotations; e.g. mock_mcp, mock_bridge, register_tools, tool_decorator, get_bridge, and _generated. As per coding guidelines, “Use type hints for all function parameters and return values in Python” and “Use camelCase for variable names in Python code.”
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 1183-1183: The use of compile can be insecure
Context: compile(code, "", "exec")
Note: [CWE-94] Improper Control of Generation of Code ('Code Injection').
(no-compile)
🤖 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/unit/test_tools_partdesign.py` around lines 1140 - 1185, Rename the
listed non-test helpers and fixtures to camelCase—mockMcp, mockBridge,
registerTools, toolDecorator, getBridge, and _generated—and add type annotations
for every parameter and return value in those functions. Update all references
consistently while preserving the existing fixture setup and generated-code
validation behavior.
Source: Coding guidelines
Comment on lines
+1196
to
+1201
| assert 'hasattr(pad, "SideType")' in code | ||
| assert 'pad.SideType = "Symmetric" if _symmetric else "One side"' in code | ||
| assert 'elif hasattr(pad, "Midplane")' in code | ||
| assert "pad.Midplane = _symmetric" in code | ||
| # SideType must be tried first, or 1.1 emits a deprecation warning. | ||
| assert code.index("SideType") < code.index("Midplane") |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert every fallback branch and the actual single binding.
These tests only verify the preferred branches and that _symmetric exists. Assert each *.Symmetric = _symmetric fallback and use code.count("_symmetric =") == 1; otherwise regressions in legacy compatibility or duplicate bindings still pass.
Also applies to: 1215-1230
🤖 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/unit/test_tools_partdesign.py` around lines 1196 - 1201, Strengthen the
relevant assertions in the test covering pad symmetry compatibility: verify
every fallback branch assigns each supported object’s Symmetric property from
_symmetric, and replace the current presence check with code.count("_symmetric
=") == 1 to enforce a single binding. Preserve the existing
SideType-before-Midplane ordering assertion and apply the same checks to the
additional branch covered around lines 1215–1230.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #94.
Summary
pad_sketch(symmetric=...)raisedAttributeErrorbecausePartDesign::Padhas noSymmetricproperty. Two things turned out to be true beyond the original report:revolution_sketchandgroove_sketchare broken the same way —rev.Symmetricandgroove.Symmetricat two further sites, neither mentioned in pad_sketch crashes on FreeCAD 1.1.1 — uses removed 'Symmetric' property (renamed 'Midplane') #94.Midplaneis not the right target on 1.1 either. FreeCAD 1.1 supersedes it withSideTypeand warns wheneverMidplaneis set:That warning lands in the console
get_console_outputreturns, so it pollutes the channel callers use for debugging. And sinceMidplaneis slated for removal, aMidplane-only fix would eventually fall through topad.Symmetric— an attribute no current version has — turning a deprecation into a hard failure later.Worth noting for the record:
git log -S "pad.Symmetric"shows it arrived in the initial commit and was never modified, so this was never a 1.1 rename —Symmetricwas simply always wrong for Pad.Approach
The generated code probes for the property instead of assuming one, so a single build works across versions and stays quiet on 1.1+:
RevolutionandGroovehave noSideTypeon 1.1.3 and emit no deprecation warning, so they use theMidplanechain only. I verified that specifically rather than applyingSideTypeuniformly.The boolean is bound to a local in the generated code so each branch reads one consistently-evaluated value.
Verification
Against live FreeCAD 1.1.3 through the xmlrpc bridge:
pad_sketch(length=6, symmetric=True)→SideType == "Symmetric", solid spansz = -3 .. +3,Shape.isValid()true. PreviouslyAttributeError.SideTypeis taken first.revolution_sketch/groove_sketchconfirmed to have noSideType, soMidplaneremains correct for them.Unit tests: 425 passed (420 on
main+ 5 added).ruff checkandruff format --checkclean.Test Plan
uv run pytest tests/unit— 425 passedruff check src/ tests/cleanruff format --check src/ tests/cleanexec'd inside FreeCAD)Midplanefallback path — I only have 1.1.3, so that branch is exercised by the property probe but not by me on a real 1.0 installNotes
The tests assert on the generated source string, which is how the existing suite works for these code-generating tools, plus a
compile()call so an indentation slip in an f-string template fails in CI rather than inside FreeCAD. They also assertSideTypeprecedesMidplane, which is the part that keeps the console quiet.Summary by CodeRabbit
Bug Fixes
Tests