fragment

[bragostin]

Apply boolean fragment to solids. https://dev.opencascade.org/doc/overview/html/specification__boolean_operations.html#specification__boolean_7

[jmwright]

Thanks for putting this together @bragostin

I still see commits happening. Please let me know when you're ready for it to be merged.

[bragostin]

@jmwright From my side it is okay, so if it is good enough for you let's merge!

[fedorkotov]

Great work. But I see two problems

• BOPAlgo_GlueEnum is not defined or imported. I get name 'BOPAlgo_GlueEnum' is not defined error when I pass glue=True argument to fragment(..)
• I would suggest extending testsuite a bit
  • Number of shapes and volume of each of Shapes in Compund created by fragment operation should be checked in the existing test to make sure fragment really does what it says on the tin
  • glue=True option should be exercised
  • Each variant allowed by type hint (Optional[Union["Workplane", Solid, Compound]]) of toFragment argument should be tested. Maybe without extensive checks of number of shapes and volume of each Shape but at least to be sure that it works at all.
  • Some example demonstrating the use of tol argument should be added

[bragostin]

@fedorkotov thank you for your feedback. Since programming is not my background I don't have a reflex for this good practice (yet). I will improve the PR along these lines.

[jmwright]

@bragostin If the changes will cause this PR to stall, we can merge and then create an issue with a checklist of the items to be done in the future. Just let me know.

[bragostin]

@fedorkotov @jmwright I have implemented all the improvements, with much more tests, except the example with tol. I was not able to imagine a relevant example. Not sure why tol is available for in the case of general fuse actually.

[fedorkotov]

I would suggest splitting one test to several test methods each of which does one thing only. This serves two purposes

• if something breaks in the future (due to incompatible changes to CadQuery for example) it is immediately obvious what exactly
• simple tests doing one thing are easier to understand and to use as a supplement for documentation

This is what I mean (see attached file). feel free to correct/expand/rename if I failed to capture some of the scenarios that your single test method tested for test_fragment.py.txt

[fedorkotov]

@bragostin It seems you haven't applied black formatting before uploading or you are not using the same version of it as automatic lint uses (19.10b0 currently, see https://raw.githubusercontent.com/CadQuery/cadquery/master/environment.yml)

[bragostin]

@fedorkotov thank you very much for providing a correct example of tests. I have run black and uploaded the new file.

[fedorkotov]

@bragostin Thank you for this contribution. I have no further change requests.

[bragostin]

@fedorkotov thank you very much for the review and the help! @jmwright then I supposed it can be merged?

[jmwright]

Merged. Thanks for all your work on this @bragostin