Add mirror from face features (with example now fixed)

[blooop]

I noticed and fixed an error in the example documentation (although this does not seem to be the cause of the failing CI from the last request)

Old pull request message:
Hi,

Apologies if I have not some something correctly, this is my first pull request so still working out how to do things.

I added this feature after asking a question on in the cadquery github page:
#521

The featured adds support for mirroring about a selected face and adds an option to perform a union as part of the mirror operation. When mirroring about a selected face I think that in most cases a mirror union will be the desired operation rather than leaving two touching but unconnected bodies.

I have added some tests and updated the examples on how to mirror about a face. In the tests, I perform checks against the bounding box, but I think checking the volume would be better. I wasn't able to to work out how to call the method that returns the volume.

Please let me know if I have done anything incorrectly or need to add anything

Thanks,
Austin

[adam-urbanczyk]

Thanks, we'll take a look but you'll have to have a little bit of patience. BTW: no need to open a new PR, you can just push additional commits.

[jmwright]

Apologies if I have not some something correctly, this is my first pull request so still working out how to do things.

@blooop No worries. We appreciate your willingness to contribute this PR, and will work through it with you.

As far as the volume, I lifted a snippet of code out of your tests and produced the following (run in CQ-editor and thus the log statement).

import cadquery as cq
b2 = cq.Workplane().box(1, 1, 1)
b2 = b2.mirror("XY", (0, 0, 0.5))
bbBox = b2.findSolid().Volume()
log(bbBox)
show_object(b2)

[blooop]

@jmwright thanks, I was trying lower case .volume(). I've updated the tests to include volume checks now.

I have added several fixes, but I cannot fix the mypy errors. It complains because I don't have a Workplane type in the union, but when I added it, the Workplane type is not recognised by mypy.

[jmwright]

@blooop It could be a similar problem to what is outlined here. This was the fix that Adam implemented

@adam-urbanczyk Will have more insight.

[blooop]

@jmwright yes, it looks like a similar issue, although I don't think the fix you linked will work in this case. mypy is correct in wanting the Workplane type in the union of possible input types, but doesn't recognise Workplane as type. I did try using CQObject as the type, but that also fails to match the type requirements.

I think the fix is to get mypy to recognise Workplane as a valid type. I have never used mypy before so I'm not sure of a solution. My basic reading around the subject implies that Workplane should be automatically detected as a type.

That said I still need to try implementing the fix you linked to see if it helps.

[adam-urbanczyk]

I think that you need to use isinstance checks for all if/else branches and part of your type handling logic does not add up. Shall I edit the code for you?

[blooop]

Yes please. I'm not sure I would know exactly what changes are needed.

…

On Tue, 1 Dec 2020, 17:31 Adam Urbańczyk, ***@***.***> wrote: I think that you need to use isinstance checks for all if/else branches and part of your type handling logic does not add up. Shall I edit the code for you? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#527 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJIHWDDNMTXZP6A7CFKKRTSSUR5JANCNFSM4UHQ53HA> .

[blooop]

Thanks for the fix, I would have missed the .toDir() function. I think my point still stands that Workplane needs to be added to the mypy type union to get rid of those errors.

[adam-urbanczyk]

It's not finished yet @blooop

[codecov]

Codecov Report

Merging #527 (bc3ad1d) into master (cf275b0) will increase coverage by 0.18%.
The diff coverage is 95.00%.

@@            Coverage Diff             @@
##           master     #527      +/-   ##
==========================================
+ Coverage   93.94%   94.13%   +0.18%     
==========================================
  Files          30       30              
  Lines        6115     6206      +91     
  Branches      640      655      +15     
==========================================
+ Hits         5745     5842      +97     
+ Misses        235      225      -10     
- Partials      135      139       +4     

Impacted Files	Coverage Δ
cadquery/occ_impl/shapes.py	91.92% <63.63%> (+0.59%)	⬆️
tests/test_workplanes.py	99.30% <98.48%> (-0.70%)	⬇️
cadquery/cq.py	89.35% <100.00%> (+0.43%)	⬆️

---

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 cf275b0...bc3ad1d. Read the comment docs.

[adam-urbanczyk]

BTW: sorry for changing the arg name - I will fix it later.

[blooop]

Yaaay! it passes all the checks!

No problem about the rename, I personally like the new one more anyway. To me the vector suffix implied a direction vector when its more of an origin/offset. (I know technically that the offset point is a vector as well).

As we are on the subject "planeOrigin" is another option for that variable name.

[adam-urbanczyk]

Is this ready now?

[blooop]

If you are happy with it then yes. Thank you for your all your help in fixing the PR to pass the checks.

[jmwright]

@blooop Looks good, thanks for doing this. Your tests look very thorough, that's great.

[adam-urbanczyk]

Merging, thanks @blooop !