-
Notifications
You must be signed in to change notification settings - Fork 61
BREAKING: Change polygon to keyword args #6385
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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
CodSpeed Instrumentation Performance ReportMerging #6385 will not alter performanceComparing Summary
|
2da9247
to
65072e6
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6385 +/- ##
==========================================
- Coverage 85.11% 85.10% -0.01%
==========================================
Files 108 108
Lines 46327 46324 -3
==========================================
- Hits 39432 39426 -6
- Misses 6895 6898 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
num_sides = { docs = "The number of sides in the polygon", include_in_snippet = true }, | ||
center = { docs = "The center point of the polygon", include_in_snippet = true }, | ||
inscribed = { docs = "Whether the polygon is inscribed (true, the default) or circumscribed (false) about a circle with the specified radius", include_in_snippet = true }, | ||
tag = { docs = "Create a new tag which refers to this line" }, |
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.
Not really related to this PR, but: afaict from the body of the function, this sets the tag on every line in the polygon sketch. Probably not what we want! Should this even take a tag param at all?
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.
WTF. It should probably not allow a tag. It might be okay if the tag could refer to the entire path, but I don't think we support that yet. I'll look into on Monday.
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.
Yeah. I think we should just remove this tag.
In the future we could consider allowing optional tags like (tagFace1 = $first, tagFace4 = $last)?
Before
After