Feature: Polar Array #301
Feature: Polar Array #301
Conversation
|
✅ Build cadquery 1.0.92 completed (commit dcef6fc86d by @bweissinger) |
Codecov Report
@@ Coverage Diff @@
## master #301 +/- ##
==========================================
+ Coverage 92.81% 92.82% +<.01%
==========================================
Files 10 10
Lines 2213 2230 +17
==========================================
+ Hits 2054 2070 +16
- Misses 159 160 +1
Continue to review full report at Codecov.
|
cadquery/cq.py
Outdated
| """ | ||
|
|
||
| if radius <= 0: | ||
| raise ValueError("Radius must be > 0 ") |
There was a problem hiding this comment.
A negative radius would work just fine, and may be a result of figuring out geometry mathematically.
There was a problem hiding this comment.
Yep, I agree it should go. I suppose I just didn't like the idea of a filthy negative radius...
cadquery/cq.py
Outdated
| 4 elements will set elements at 0, 30, 60, and 90 as expected. Angle / | ||
| count would place elements at 0, 22.5, 45, and 67.5. If fill angle | ||
| is a full circle, the correction is not needed. | ||
| """ |
There was a problem hiding this comment.
This won't be added to the docstring, should this be a comment?
cadquery/cq.py
Outdated
| raise ValueError("Angle cannot be 0") | ||
|
|
||
| if startAngle < -360 or startAngle > 360: | ||
| raise ValueError("Start angle must be in range: -360 to 360") |
There was a problem hiding this comment.
Why? similar to negative radius restriction
There was a problem hiding this comment.
Completely correct, that does not need to be there.
|
@fragmuffin brought up some good points. I think the only restriction that needs to stay is the count, although maybe change that to allow for a single element array? The logic for fill angle calculation should also be changed to a more general form. Why don't I go back and make some tweaks and clean things up a bit. |
|
✅ Build cadquery 1.0.93 completed (commit 52cc012011 by @bweissinger) |
|
@bweissinger It looks like the comments from @fragmuffin have been addressed. Is this ready to merge? |
|
This looks great to me! |
|
I believe so. |
|
@bweissinger Thanks for another great contribution! |
Adds #299