Skip to content
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

Merged
merged 29 commits into from
Sep 26, 2021
Merged

until extrude/cutblind #875

merged 29 commits into from
Sep 26, 2021

Conversation

Jojain
Copy link
Contributor

@Jojain Jojain commented Aug 31, 2021

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.

@jmwright
Copy link
Member

jmwright commented Sep 1, 2021

@Jojain If you run black on this locally does it pass? CI is complaining.

@Jojain
Copy link
Contributor Author

Jojain commented Sep 1, 2021

@jmwright
I ran it before sending the PR.
Maybe not from top of the repo though I'll take a look tonight.

@Jojain
Copy link
Contributor Author

Jojain commented Sep 1, 2021

@jmwright is there a specific version of black to use ?
Appveyor was saying he would reformat 6 files, my black run reformatted 4 files and now Appveyor says he would reformat 9 files 🙄

@jmwright
Copy link
Member

jmwright commented Sep 1, 2021

Yes, the version of black is pinned for CI. Please see this source.

@codecov
Copy link

codecov bot commented Sep 1, 2021

Codecov Report

Merging #875 (4fb78ce) into master (d8ba84e) will increase coverage by 0.03%.
The diff coverage is 96.82%.

❗ Current head 4fb78ce differs from pull request most recent head 8dab694. Consider uploading reports for the commit 8dab694 to get more accurate results
Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
cadquery/assembly.py 93.13% <ø> (ø)
cadquery/cqgi.py 79.09% <ø> (ø)
cadquery/occ_impl/importers.py 88.42% <ø> (ø)
tests/test_selectors.py 100.00% <ø> (ø)
cadquery/cq.py 91.40% <93.75%> (+0.05%) ⬆️
cadquery/occ_impl/shapes.py 92.70% <95.45%> (+0.22%) ⬆️
cadquery/occ_impl/geom.py 99.57% <100.00%> (+<0.01%) ⬆️
tests/test_cad_objects.py 99.38% <100.00%> (+0.01%) ⬆️
tests/test_cadquery.py 99.23% <100.00%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a94606...8dab694. Read the comment docs.

@Jojain
Copy link
Contributor Author

Jojain commented Sep 1, 2021

It's back to where it was know with only coverage failing.
Codecov complains about some errors raising not tested, however most of them don't need tests in my opinion as they are just checks on wrong parameters combination.

I'll be adding docs and examples about this once it's merged and there is no API change

@adam-urbanczyk
Copy link
Member

Thanks, I still see though the unwanted test files being committed. Let me check how to get rid of those.

cadquery/cq.py Show resolved Hide resolved
cadquery/occ_impl/shapes.py Outdated Show resolved Hide resolved
@adam-urbanczyk
Copy link
Member

It's back to where it was know with only coverage failing.
Codecov complains about some errors raising not tested, however most of them don't need tests in my opinion as they are just checks on wrong parameters combination.

It'd be good to trigger those errors in tests.

@adam-urbanczyk
Copy link
Member

One more file to go: tests/test.brep

@Jojain
Copy link
Contributor Author

Jojain commented Sep 8, 2021

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

@jmwright
Copy link
Member

jmwright commented Sep 8, 2021

Wouldn't it be nice to add those files in the .gitignore ?

The .gitignore on master has these files. It was a recent merge.

@adam-urbanczyk
Copy link
Member

@marcus7070 do you plan to take a look?

@marcus7070
Copy link
Member

Yes, I'll try to get it finished in the next few hours.

Copy link
Member

@marcus7070 marcus7070 left a 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/cq.py Outdated Show resolved Hide resolved
cadquery/cq.py Outdated Show resolved Hide resolved
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
:param basis: face to perfrom the operation on
:param basis: face to perform the operation on

cadquery/occ_impl/shapes.py Outdated Show resolved Hide resolved
cadquery/occ_impl/shapes.py Outdated Show resolved Hide resolved
cadquery/occ_impl/shapes.py Outdated Show resolved Hide resolved
cadquery/cq.py Show resolved Hide resolved
cadquery/occ_impl/shapes.py Show resolved Hide resolved
cadquery/cq.py Show resolved Hide resolved
cadquery/cq.py Show resolved Hide resolved
@Jojain
Copy link
Contributor Author

Jojain commented Sep 20, 2021

@marcus7070
I think I'm done, let me know if there is things I forgot/haven't responded !

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
@marcus7070
Copy link
Member

@Jojain can you have a look at a91f623 and 20691bc and make sure you understand and agree with what I've done there? Hopefully the tests make it obvious why I'm making those changes.

@Jojain
Copy link
Contributor Author

Jojain commented Sep 25, 2021

Thanks @marcus7070 it's now obvious what you meant about limitFace = facesList[upToFace] and I had missed it.
That's all good for me. I'll try to bring another PR in the near future to add some doc

@marcus7070 marcus7070 merged commit f19c35c into CadQuery:master Sep 26, 2021
@marcus7070
Copy link
Member

Thank you very much @Jojain, this will be a great addition!

@jmwright, note that we've renamed the argument for Workplane.extrude from distance to until and Workplane.cutBlind's distanceToCut also to until. I think that needs to be mentioned in the changelog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants