-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KCL: Sweep stdlib fn now uses keyword args #5300
Conversation
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
ad468ed
to
8f0a43c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you'll need to fix the point-and-click tests for Sweep, see
modeling-app/e2e/playwright/point-click.spec.ts
Lines 967 to 968 in 5dc4213
const sweepDeclaration = 'sweep001 = sweep({ path = sketch002 }, sketch001)' | |
const declaration = createVariableDeclaration(name, sweep) | ||
modifiedAst.body.push(declaration) | ||
const pathToNode: PathToNode = [ | ||
['body', ''], | ||
[modifiedAst.body.length - 1, 'index'], | ||
['declaration', 'VariableDeclaration'], | ||
['init', 'VariableDeclarator'], | ||
['arguments', 'CallExpression'], | ||
[0, 'index'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was surprised that this referred to the SweepData
object (e.g. {path: myPath}
). I thought it would refer to the sketch being swept.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I had messed this one up?
8f0a43c
to
8af3a8d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to update the docs after this change.
args = { | ||
sketch = { docs = "The sketch that should be swept in space" }, | ||
path = { docs = "The path to sweep the sketch along" }, | ||
sectional = { docs = "If true, the sweep will be broken up into sub-sweeps (extrusions, revolves, sweeps) based on the trajectory path components." }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note here: sectional is missing from point-and-click implementation today, created an issue to track it #5301 (v1 scope creep candidate labeled ofc!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, tested locally. Hope we won't need to babysit the tests too much to get it merged! I see the snapshots are acting up
e5bfa56
to
7e131d9
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5300 +/- ##
==========================================
- Coverage 86.04% 86.04% -0.01%
==========================================
Files 92 92
Lines 33254 33250 -4
==========================================
- Hits 28613 28609 -4
Misses 4641 4641
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
7e131d9
to
f4d5c2d
Compare
Before:
After: