search

CadQuery / cadquery

A python parametric CAD scripting framework based on OCCT
https://cadquery.readthedocs.io
Other
3.15k stars 289 forks source link

Fix union with None #1560

Closed adam-urbanczyk closed 5 months ago

adam-urbanczyk commented 5 months ago

Closes #1536

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 94.54%. Comparing base (3451007) to head (926212a).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1560 +/- ## ======================================= Coverage 94.54% 94.54% ======================================= Files 28 28 Lines 5846 5848 +2 Branches 1165 1166 +1 ======================================= + Hits 5527 5529 +2 Misses 193 193 Partials 126 126 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

michaelgale commented 5 months ago

Wow! Really grateful that this has finally been changed. I can't tell you how much boilerplate code I've written to handle the inevitable union to "None" scenario when making composite solids/geometries from iteration.

adam-urbanczyk commented 5 months ago

Thanks @michaelgale ! Was there something stopping you from opening an issue or PR? Or did we miss it somehow?

michaelgale commented 5 months ago

Thanks @michaelgale ! Was there something stopping you from opening an issue or PR? Or did we miss it somehow?

I am somewhat hesitant to suggest changes to a mature core API since these types of changes can have unpredictable consequences for other users. Since this issue didn't represent a bug or unexpected behaviour, I didn't think to ever raise a PR or issue. Since CQ is so easily extended with plugins, I tend to rely on this mechanism to introduce syntactic shortcuts and convenience functions to streamline my integrations with CQ. In future, I might lower my threshold for raising issues and/or contributing changes with a PR. Thanks again to the CQ team for maintaining this project.

adam-urbanczyk commented 5 months ago

@lorenzncode I'm not sure that I understand. Currently union with None results in an error.

jmwright commented 5 months ago

Looks good, thanks @adam-urbanczyk

lorenzncode commented 5 months ago

@adam-urbanczyk Right, I'm only pointing out that skipping union to avoid the error is not exactly the same as this fix.

In #1536 , union2 is defined to avoid the error by skipping the call to union.

In the PR branch, calling union with None extends the Workplane chain and does not result in error.

The result at the end of the chain is the same in either case only that the length of the chain is different.

adam-urbanczyk commented 5 months ago

@adam-urbanczyk Right, I'm only pointing out that skipping union to avoid the error is not exactly the same as this fix.

In #1536 , union2 is defined to avoid the error by skipping the call to union.

That is the intention, .union() will in general union items on the stack, so creating a new object is desired. Shall I merge?

lorenzncode commented 5 months ago

That makes sense to me. +1 to merge. Thanks @adam-urbanczyk