Repository navigation
Conversation
The path editing and drawing commands read their optional numbers with
f64_or or as_f64, which fall back to a default for a value that is not
a JSON number. path.reshapeSegment {"dy": "10"} bent the segment by 0
and reported success, and object.blend.spine.reshapeSegment did the
same with t, dx and dy. path.convertAnchor converted an anchor without
a handle position when x or y was a string or only one of them was
given. path.curvatureEdit and path.insertAnchor inserted at the middle
of the segment for a t given as text, and path.curvatureEdit left an
inserted anchor where it was when x and y were strings. path.freehand
used 1.5 pt for a fidelity given as text and 12 pt for such a redraw
within. The Eraser, Path Eraser, Smooth, Join and Blob Brush commands
used their default size, width, radius, fidelity or tolerance, and
path.moveAnchors and path.reshape did the same with dx, dy, tol and
radius. The drawing commands left out a point whose x or y was not a
number, path.freehand drew a point at full pressure when its pressure
was text, and path.close closed a path without its handle when in was
not an array starting with two numbers, and used the first two numbers
of a longer array.
These commands now fail with "`<key>` must be a number, not <value>"
for a value that is not a number or null, as the transform, blend and
Object menu commands do. The document does not change and no undo
step is recorded. path.convertAnchor refuses an x without a y, or a y
without an x, with "x and y must be given together", the message
object.blend.spine.convertAnchor gives. A point whose x or y is not a
number is refused with "x and y must both be numbers", and path.close
refuses an in that is not [x, y] with "`in` must be [x, y]".
path.close also refuses an in of more than two numbers.
Docs: docs/mcp.md, docs/target-app-parity.md.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 35 functions across 5 files. (2 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: path anchor and drawing commands now reject numeric values provided as text. Description check The description includes the required What and why, How it was tested, and Checklist sections. It explains the behavior changes, documents regression coverage, reports validation results, and marks al… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description includes the required What and why, How it was tested, and Checklist sections. It explains the behavior changes, documents regression coverage, reports validation results, and marks all checklist items complete.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 35 functions across 5 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
🔧 LanguageTool
-
docs/target-app-parity.md
-
LanguageTool checks are incomplete because the per-file request limit of 5 was reached. Remaining text was skipped; findings from completed checks are retained.
-
Comment @coderabbitai help to get the list of available commands.
FredApps
left a comment
FredApps
left a comment
There was a problem hiding this comment.
Reviewed at 705601e with FreeBSD x86_64 build and test green. The path and drawing commands now refuse a point, pressure, size, width, radius, fidelity, tolerance, t or delta that is not a finite number (naming the key) instead of silently dropping the point or using the default, null still reads as the key left out, path.convertAnchor requires x and y together, path.close validates its in handle, and every refusal leaves the document and undo history untouched; the shared point_entry helper and the per-command check_numbers calls are consistent, and the tests and docs/mcp.md cover each command.
This branch has not been deployed
2 participants
What and why
On
main, the path editing and drawing commands read their optional numbers withf64_ororas_f64, which fall back to a default for a value that is not a JSON number.path.reshapeSegment {"dy": "10"}bends the segment by 0 and reports success,path.convertAnchor {"to": "smooth", "x": "200", "y": "150"}converts the anchor without a handle position, andpath.freehand {"redraw": {"within": "4"}}redraws within 12 pt.path.reshapeSegmentandobject.blend.spine.reshapeSegment(t,dx,dy),path.curvatureEdit(x,y,t),path.convertAnchor(x,y),path.insertAnchor(t),path.freehand(fidelityand thewithinofredraw),path.eraseRegionandpath.blob(size),path.eraseSegments(width),path.smoothRegion(radius,fidelity),path.joinScrub(tolerance),path.moveAnchors(dx,dy) andpath.reshape(dx,dy,tol,radius) fail with "<key>must be a number, not " for a value that is not a number or null, as the transform, blend and Object menu commands do since Transform commands: a string for an optional number is an error (#1952) #1967, Blends, meshes and envelopes: a number given as text is refused #2851 and Transform Each, Rasterize, Mosaic, Trim Marks and Rearrange: a number given as text is refused #2917 (check_numbers,crates/engine/src/cmd/mod.rs:323). The document and the undo history stay as they were.path.convertAnchorrefuses anxwithout ay, or aywithout anx, with "x and y must be given together", the messageobject.blend.spine.convertAnchorgives. Before, it converted the anchor without a handle position.path.freehand,path.curvature,path.knife,path.eraseRegion,path.eraseSegments,path.smoothRegion,path.joinScrubandpath.blobrefuse a point whose x or y is not a number with "x and y must both be numbers", and all of them exceptpath.curvaturerefuse a pen pressure that is neither a number nor null with "pressuremust be a number, not ". Before, they left such a point out, andpath.freehanddrew a point with such a pressure at full pressure.path.closerefuses aninthat is not an array of exactly two numbers with "inmust be [x, y]". Before, it closed the path without the handle, or used the first two numbers of a longer array.docs/mcp.mdlists the keys, anddocs/target-app-parity.mdhas one line for the change. The 19 catalogs that translate every status and error message, and bg, already have rows for "{key}must be a number, not {v}", "x and y must both be numbers" and "{k}must be [x, y]"; the change adds no catalog rows. "x and y must be given together" has no rows there, as forobject.blend.spine.convertAnchor.How it was tested
New tests in vectorcraft-engine, each failing on
main, where the first call succeeds:tests_draw2::reshape_segment_refuses_a_number_given_as_textgivest,dxanddyas a string, an array ortrue; the path stays as it was, anddy: 20witht: nullstill bends it.tests_blendspine::spine_reshape_segment_refuses_a_number_given_as_textdoes the same for a blend's spine.tests_draw2::convert_anchor_refuses_a_handle_position_given_as_text_or_alonegivesxandyas text and each one alone;x: 330, y: 150still places the out handle there.tests_draw2::curvature_edit_and_insert_anchor_refuse_a_number_given_as_textgives the Curvature insert and move edits at,xoryas text, andpath.insertAnchoratas text.tests_draw2::pencil_refuses_a_fidelity_or_within_given_as_text: no path is drawn or redrawn, and nullfidelityandwithinredraw the line.tests_draw2::drawing_commands_refuse_a_point_or_pressure_given_as_textgives the eight point commands a point with a string x or y, a point written as a string, a point of one number andnull, and gives the seven that take a pressure one as text.tests_draw2::drawing_commands_refuse_a_size_given_as_textgives each size, width, radius, fidelity and tolerance as text; a nullsizeerases with the default 10 pt.tests_draw2::move_anchors_reshape_and_close_refuse_a_number_given_as_textgivespath.moveAnchorsandpath.reshapetheir numbers as text, andpath.closefourinvalues that are not[x, y].Targeted checks while building:
cargo clippy -p vectorcraft-engine --all-targets -- -D warnings; the new tests;cargo test -p vectorcraft-engine --liband--test command_sweep;cargo fmt --all -- --check;cargo xtask brands,layersandassets.On Windows 11 (x86-64) with rustc 1.99.1, on top of
mainatbc1ae08:CRAFT_FONTS_DIR=<craft-fonts checkout> cargo xtask cipasses its 10 steps: 8,578 tests passed, 0 failed, 13 ignored.git merge-treereports no conflicts withmainatbc1ae08, or with #2910, which also changespath.freehand. #2592, #613, #2539 and #2071 also changedraw2.rs,path.rsorlive.rs, and #613, #1868 and #3053 changetests_draw2.rs; their conflicts withmainare the same with or without this branch.cargo check -p vectorcraft-engine --testspasses on the branch merged with #2910.Checklist
cargo xtask cipasses (fmt, clippy, tests, assets, brands, layers, wasm).unwrap(),expect(),panic!,unreachable!,todo!orunimplemented!outside tests, and no unchecked indexing on data from files, parameters ormessages. Errors are returned as
Resultand shown to the user(why and how).
ASSETS.md.Summary by CodeRabbit
Bug Fixes
null.nullcontinues to use defaults or omit optional values.Documentation