fix: strip parentheses from origin parameter in string parser#569
Open
claw-explorer wants to merge 1 commit intotscircuit:mainfrom
Open
fix: strip parentheses from origin parameter in string parser#569claw-explorer wants to merge 1 commit intotscircuit:mainfrom
claw-explorer wants to merge 1 commit intotscircuit:mainfrom
Conversation
The string parser passes origin values with parentheses (e.g., "(pin1)")
when using the fp.string("dip8_origin(pin1)") syntax. However,
applyOrigin() expects bare values like "pin1", "center", etc.
This caused all origin modes to silently fail when used in footprinter
strings - pads were not repositioned even though origin was specified.
The builder API (fp().dip(8).origin("pin1")) was unaffected because
it passes bare strings directly.
Fix: normalize parenthesized values in applyOrigin before matching.
Tests: 9 new tests covering all origin modes through the string parser,
parity with builder API, and silkscreen shift verification.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Comment on lines
+1
to
+113
| import { test, expect } from "bun:test" | ||
| import { fp } from "../src/footprinter" | ||
|
|
||
| test("origin(pin1) works in string parser for dip8", () => { | ||
| const circuit = fp.string("dip8_origin(pin1)").circuitJson() | ||
| const pad1 = circuit.find( | ||
| (el: any) => | ||
| el.type === "pcb_plated_hole" && | ||
| (el.port_hints?.[0] === "1" || el.port_hints?.[0] === 1), | ||
| ) as any | ||
| expect(pad1).toBeDefined() | ||
| expect(pad1.x).toBeCloseTo(0) | ||
| expect(pad1.y).toBeCloseTo(0) | ||
| }) | ||
|
|
||
| test("origin(center) works in string parser for soic8", () => { | ||
| const circuit = fp.string("soic8_origin(center)").circuitJson() | ||
| const pads = circuit.filter((el: any) => el.type === "pcb_smtpad") as any[] | ||
| expect(pads.length).toBe(8) | ||
| // With center origin, the average position should be near (0, 0) | ||
| const avgX = pads.reduce((s: number, p: any) => s + p.x, 0) / pads.length | ||
| const avgY = pads.reduce((s: number, p: any) => s + p.y, 0) / pads.length | ||
| expect(avgX).toBeCloseTo(0, 1) | ||
| expect(avgY).toBeCloseTo(0, 1) | ||
| }) | ||
|
|
||
| test("origin(pin1) works in string parser for res0402", () => { | ||
| const circuit = fp.string("res0402_origin(pin1)").circuitJson() | ||
| const pad1 = circuit.find( | ||
| (el: any) => el.type === "pcb_smtpad" && el.port_hints?.[0] === "1", | ||
| ) as any | ||
| expect(pad1).toBeDefined() | ||
| expect(pad1.x).toBeCloseTo(0) | ||
| expect(pad1.y).toBeCloseTo(0) | ||
| }) | ||
|
|
||
| test("origin(bottomleft) works in string parser for res0603", () => { | ||
| const circuit = fp.string("res0603_origin(bottomleft)").circuitJson() | ||
| const pads = circuit.filter((el: any) => el.type === "pcb_smtpad") as any[] | ||
| // All pads should have non-negative x and y after bottomleft origin | ||
| for (const pad of pads) { | ||
| expect(pad.x).toBeGreaterThanOrEqual(-0.01) | ||
| expect(pad.y).toBeGreaterThanOrEqual(-0.01) | ||
| } | ||
| }) | ||
|
|
||
| test("string parser origin matches builder API origin for pin1", () => { | ||
| const fromString = fp.string("dip8_origin(pin1)").circuitJson() | ||
| const fromBuilder = fp().dip(8).origin("pin1").circuitJson() | ||
|
|
||
| const stringPads = fromString.filter( | ||
| (el: any) => el.type === "pcb_plated_hole", | ||
| ) as any[] | ||
| const builderPads = fromBuilder.filter( | ||
| (el: any) => el.type === "pcb_plated_hole", | ||
| ) as any[] | ||
|
|
||
| expect(stringPads.length).toBe(builderPads.length) | ||
| for (let i = 0; i < stringPads.length; i++) { | ||
| expect(stringPads[i].x).toBeCloseTo(builderPads[i].x) | ||
| expect(stringPads[i].y).toBeCloseTo(builderPads[i].y) | ||
| } | ||
| }) | ||
|
|
||
| test("string parser origin matches builder API origin for center", () => { | ||
| const fromString = fp.string("soic8_origin(center)").circuitJson() | ||
| const fromBuilder = fp().soic(8).origin("center").circuitJson() | ||
|
|
||
| const stringPads = fromString.filter( | ||
| (el: any) => el.type === "pcb_smtpad", | ||
| ) as any[] | ||
| const builderPads = fromBuilder.filter( | ||
| (el: any) => el.type === "pcb_smtpad", | ||
| ) as any[] | ||
|
|
||
| expect(stringPads.length).toBe(builderPads.length) | ||
| for (let i = 0; i < stringPads.length; i++) { | ||
| expect(stringPads[i].x).toBeCloseTo(builderPads[i].x) | ||
| expect(stringPads[i].y).toBeCloseTo(builderPads[i].y) | ||
| } | ||
| }) | ||
|
|
||
| test("origin(topcenter) works in string parser", () => { | ||
| const circuit = fp.string("res0805_origin(topcenter)").circuitJson() | ||
| const pads = circuit.filter((el: any) => el.type === "pcb_smtpad") as any[] | ||
| expect(pads.length).toBe(2) | ||
| // Top center means highest pad edge should be at y=0 | ||
| const maxY = Math.max(...pads.map((p: any) => p.y + p.height / 2)) | ||
| expect(maxY).toBeCloseTo(0, 1) | ||
| }) | ||
|
|
||
| test("origin(rightcenter) works in string parser", () => { | ||
| const circuit = fp.string("soic8_origin(rightcenter)").circuitJson() | ||
| const pads = circuit.filter((el: any) => el.type === "pcb_smtpad") as any[] | ||
| // Right center means rightmost pad edge should be at x=0 | ||
| const maxX = Math.max(...pads.map((p: any) => p.x + p.width / 2)) | ||
| expect(maxX).toBeCloseTo(0, 1) | ||
| }) | ||
|
|
||
| test("origin shifts silkscreen elements in string parser", () => { | ||
| const withoutOrigin = fp.string("dip8").circuitJson() | ||
| const withOrigin = fp.string("dip8_origin(pin1)").circuitJson() | ||
|
|
||
| const silkWithout = withoutOrigin.find( | ||
| (el: any) => el.type === "pcb_silkscreen_path", | ||
| ) as any | ||
| const silkWith = withOrigin.find( | ||
| (el: any) => el.type === "pcb_silkscreen_path", | ||
| ) as any | ||
|
|
||
| // Silkscreen routes should be shifted | ||
| expect(silkWith.route[0].x).not.toBeCloseTo(silkWithout.route[0].x) | ||
| }) |
Contributor
There was a problem hiding this comment.
This test file violates the rule that a *.test.ts file may have AT MOST one test(...), after that the user should split into multiple, numbered files. The file contains 9 test cases but should be split into multiple numbered files like origin-string-parser1.test.ts, origin-string-parser2.test.ts, etc. Each file should contain only one test case.
Spotted by Graphite (based on custom rule: Custom rule)
Is this helpful? React 👍 or 👎 to let us know.
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.
Problem
The
originparameter silently fails when used in footprinter strings. All origin modes are completely broken through the string parser:All 7 origin modes are affected:
center,pin1,bottomleft,bottomcenter,topcenter,leftcenter,rightcenter.Root Cause
The string parser regex captures function-call-style parameters with their parentheses intact. For example,
origin(pin1)is stored as"(pin1)"in the params object.Other function-call parameters like
missing(A1,B2)handle this through their Zodfunction_calltransform which strips parentheses. Butoriginis a simple string that goes directly toapplyOrigin(), which expects bare values like"pin1", not"(pin1)".Since
"(pin1)"matches none of the switch cases inapplyOrigin,dxanddyremain 0 and no transformation is applied.Fix
Normalize parenthesized values in
applyOrigin()before matching:This is consistent with how
function_call.tshandles the same pattern for other parameters.Tests
Added 9 tests in
origin-string-parser.test.ts:origin(pin1)for DIP, res0402origin(center)for SOICorigin(bottomleft)for res0603origin(topcenter)andorigin(rightcenter)All 393 tests pass (384 existing + 9 new).