-
Notifications
You must be signed in to change notification settings - Fork 299
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
until extrude/cutblind #875
Conversation
@Jojain If you run black on this locally does it pass? CI is complaining. |
@jmwright |
@jmwright is there a specific version of black to use ? |
Yes, the version of black is pinned for CI. Please see this source. |
Codecov Report
@@ Coverage Diff @@
## master #875 +/- ##
==========================================
+ Coverage 95.88% 95.91% +0.03%
==========================================
Files 32 32
Lines 7654 7817 +163
Branches 815 846 +31
==========================================
+ Hits 7339 7498 +159
Misses 186 186
- Partials 129 133 +4
Continue to review full report at Codecov.
|
It's back to where it was know with only coverage failing. I'll be adding docs and examples about this once it's merged and there is no API change |
Thanks, I still see though the unwanted test files being committed. Let me check how to get rid of those. |
It'd be good to trigger those errors in tests. |
One more file to go: |
Wouldn't it be nice to add those files in the .gitignore ? Each pytest run make them pop again, and I could recommit them by mistake |
The .gitignore on master has these files. It was a recent merge. |
@marcus7070 do you plan to take a look? |
Yes, I'll try to get it finished in the next few hours. |
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.
Thanks @Jojain, this was a big job and a great improvement. I have some minor points.
The multiple types in the docstrings are not working well in the HTML docs, see https://cadquery--875.org.readthedocs.build/en/875/classreference.html#cadquery.Workplane.extrude
I would suggest changing it to have no explicit :type until:
lines and instead combine all that information into the param line, like:
:param until: The distance to extrude, normal to the workplane plane. When a float is passed, the extrusion extends this far and a negative value is in the opposite direction to the normal of the plane. The string "next" extrudes until the next face orthogonal to the wire normal. "last" extrudes to the last face. If a object of type Face is passed then the extrusion will extend until this face.
cadquery/occ_impl/shapes.py
Outdated
thruAll: bool = True, | ||
additive: bool = True, | ||
) -> "Solid": | ||
""" | ||
Make a prismatic feature (additive or subtractive) | ||
|
||
:param basis: face to perform the operation on | ||
:param basis: face to perfrom the operation on |
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.
:param basis: face to perfrom the operation on | |
:param basis: face to perform the operation on |
Co-authored-by: Marcus Boyd <[email protected]>
Co-authored-by: Marcus Boyd <[email protected]>
Co-authored-by: Marcus Boyd <[email protected]>
Co-authored-by: Marcus Boyd <[email protected]>
Co-authored-by: Marcus Boyd <[email protected]>
add test for direction=None
@marcus7070 |
Line length and removing empty lines from docstrings
Added test for extruding from inside a solid up to next and changed the order of statements in Workplane.extrude
Added test for this and changed extrude to raise a ValueError
Thanks @marcus7070 it's now obvious what you meant about limitFace = |
Final PR that would be hopefully totally clean this time.
Sorry for the duplicates, I'm note very familiar with git / github so I made some mistakes along the way.