fix(#829, #877): add #595 to SEMVER.md v2.0.0 break table; remove orphaned doc comment - #939
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| Sources/OCCTBridge/include/OCCTBridge.h | ||
| Sources/OCCTBridge/src/OCCTBridge_AIS.mm | ||
| Sources/OCCTBridge/src/OCCTBridge_Curve3D.mm | ||
| Sources/OCCTBridge/src/OCCTBridge_Document.mm |
There was a problem hiding this comment.
SUGGESTION: PR scope mixes unrelated changes
This PR contains two distinct commits:
- Large clang-format compliance refactoring (OCCTBridge_Geom2d.mm, OCCTBridge_Internal.h, OCCTBridge_Topology.mm) + style manifest updates
- Actual fixes for docs/SEMVER.md's v2.0.0 break table omits #595 (curvature getters become Double?) #829 (SEMVER.md) and Two orphaned doc comments found via SwiftLint (Surface.swift, Shape+Modeling.swift) #877 (orphaned doc comment)
The PR description only mentions commit 2. Consider splitting into two PRs for clearer review and git history.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous Review Summaries (2 snapshots, latest commit 3edc4e3)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 3edc4e3)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (7 files)
Fix these issues in Kilo Cloud Previous review (commit c2d7226)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (6 files)
Reviewed by nemotron-3-ultra-550b-a55b:free · Input: 263.7K · Output: 6.1K · Cached: 467.1K |
| { | ||
| poles(i) = convPoles.Value(i); | ||
| weights(i) = convWeights.Value(i); | ||
| poles(i) = conv.Pole(i); |
There was a problem hiding this comment.
CRITICAL: Reintroduces deprecated per-index accessors — regression from #801
The comment at line 4565 says "#801: use batch accessors (Poles/Weights/Knots/Multiplicities) instead of deprecated per-index accessors (Pole/Weight/Knot/Multiplicity)" but the code change does the exact opposite: it switches FROM batch accessors TO deprecated per-index accessors.
Old (correct, per #801):
const TColgp_Array1OfPnt2d& convPoles = conv.Poles();
const TColStd_Array1OfReal& convWeights = conv.Weights();
const TColStd_Array1OfReal& convKnots = conv.Knots();
const TColStd_Array1OfInteger& convMults = conv.Multiplicities();
for (int i = 1; i <= np; i++) {
poles(i) = convPoles.Value(i);
weights(i) = convWeights.Value(i);
}
for (int i = 1; i <= nk; i++) {
knots(i) = convKnots.Value(i);
mults(i) = convMults.Value(i);
}New (regression - uses deprecated API):
for (int i = 1; i <= np; i++) {
poles(i) = conv.Pole(i);
weights(i) = conv.Weight(i);
}
for (int i = 1; i <= nk; i++) {
knots(i) = conv.Knot(i);
mults(i) = conv.Multiplicity(i);
}This reverts the fix from #801 which explicitly migrated to batch accessors because the per-index accessors are deprecated.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| Sources/OCCTBridge/src/OCCTBridge_ProjLib_NLPlate.mm | ||
| Sources/OCCTBridge/src/OCCTBridge_Properties.mm | ||
| Sources/OCCTBridge/src/OCCTBridge_Spatial.mm | ||
| Sources/OCCTBridge/src/OCCTBridge_Surface.mm |
There was a problem hiding this comment.
WARNING: Manifest policy violation — adds file to exemption list instead of removing it
Sources/OCCTBridge/src/OCCTBridge_Surface.mm is added to the exemption list. Per Scripts/check-style-manifest.py and okf/policies/code-style.md: "A manifest may only shrink. Comparing HEAD's manifest against the base ref's, any entry present at HEAD but absent at the base is a new grandfather-listing, which this policy doesn't allow: everything new complies from creation."
If the file was reformatted to be compliant, it should be REMOVED from the manifest (not added). If it's not compliant, the PR shouldn't touch it without bringing it into compliance first.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| Sources/OCCTSwift/Shape+Geom2d.swift | ||
| Sources/OCCTSwift/Shape+Math.swift | ||
| Sources/OCCTSwift/Shape+Mesh.swift | ||
| Sources/OCCTSwift/Shape+Modeling.swift |
There was a problem hiding this comment.
WARNING: Manifest policy violation — adds file to exemption list instead of removing it
Sources/OCCTSwift/Shape+Modeling.swift is added to the exemption list. Per Scripts/check-style-manifest.py and okf/policies/code-style.md: "A manifest may only shrink. Comparing HEAD's manifest against the base ref's, any entry present at HEAD but absent at the base is a new grandfather-listing, which this policy doesn't allow: everything new complies from creation."
The PR performs massive reformatting on this file. If the reformatting brings it into compliance, the file should be REMOVED from the manifest. Adding it to the manifest indicates it's NOT compliant, which contradicts the purpose of the reformatting.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| /// - transition: How to handle transitions at spine corners | ||
| /// - solid: If true, create a solid; if false, create a shell | ||
| /// - Returns: Swept shape, or nil on failure | ||
| // MARK: - Variable-Section Sweep (v0.21.0) |
There was a problem hiding this comment.
WARNING: PR scope creep — massive reformatting not described in PR description
The PR description states "Two minor issues: #829 SEMVER.md omission and #877 orphaned doc comment" but this file contains hundreds of lines of reformatting across dozens of functions (changing multi-line guards to single-line, compacting function signatures, combining variable declarations). This is a large refactoring, not a minor doc fix.
Per okf/policies/changelog-on-merge.md and standard practice, PRs should do one logical thing. Mixing a massive style refactoring with targeted doc fixes makes review difficult and conflates unrelated changes.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
…haned doc comment - docs/SEMVER.md: add v2.0.0 break table entry for #595 (six curvature getters now return Double? instead of Double — compile error for callers) - Sources/OCCTSwift/Shape+Modeling.swift: remove orphaned doc comment (duplicate of pipeShell's doc comment) before MARK line All 6 gate scripts pass, Modeling tests (647) pass.
3edc4e3 to
83e74a2
Compare
2 participants
What & why
Two minor issues:
#829: SEMVER.md v2.0.0 break table omits #595
The v2.0.0 break table listed 17 entries but was missing #595 ("Six more curvature getters spell 'undefined' as 0, on Curve3D, Curve2D, Surface and Shape"). The fix changed six curvature getters from returning
Double(with0meaning undefined) to returningDouble?(withnilmeaning undefined). This is a compile error for any caller — the migration is to unwrap or coalesce to0.Added a new v2.0.0 subsection covering all six affected getters:
Curve2D.curvature(at:)Curve3D.curvature(at:)Curve3D.localCurvature(at:)Surface.gaussianCurvature(atU:v:)Surface.meanCurvature(atU:v:)Shape.edgeCurvatureLP(at:)#877: Two orphaned doc comments found via SwiftLint
Sources/OCCTSwift/Shape+Modeling.swift:405— A doc comment ("Create a pipe shell with transition mode control...") sat orphaned before// MARK: - Variable-Section Sweep (v0.21.0). This was pure duplication:pipeShell(spine:profile:mode:...)(line ~391) already has its own complete doc comment covering the sametransitionparameter verbatim. Fix: deleted the orphaned block.Sources/OCCTSwift/Surface.swift:1329— The issue reported a doc comment ("Convert this surface to an array of Bezier surface patches...") orphaned before// MARK: - Surface Singularity Analysis (v0.37.0). This appears to have been fixed in a prior commit — the doc comment now correctly sits directly abovetoBezierPatches()(line 1588). No change needed.Verification
swift build --target OCCTModelingTests: passesswiftlint linton both files: no orphaned doc commentsSemVer impact
PATCH. Documentation correction only — no code behavior change (the #595 API change was already released in v2.0.0; this just documents it in SEMVER.md).