Fix union with None

[adam-urbanczyk]

Closes #1536

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

@@           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           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[michaelgale]

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]

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

[michaelgale]

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.

[lorenzncode]

Hey just one comment regarding boilerplate. This behavior results in additional parents which may be different than current boilerplate. As @michaelgale mentions iteration, I suppose it depends on size of the iteration or sparseness whether there is any significant noticeable impact.

import cadquery as cq

r0 = cq.Workplane().cylinder(1, 10)

for i in range(1, 101):
    if i == 50:
        obj = cq.Workplane().box(1, 1, 10)
    else:
        obj = None

    r0 = r0.union(obj)

[adam-urbanczyk]

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

[jmwright]

Looks good, thanks @adam-urbanczyk

[lorenzncode]

@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]

@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]

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