Alignment rework

[erooke]

While using build123d I noticed that the type signature for BoundBox.to_align_offset was wrong. While fixing it I noticed that there was a lot of replicated aligning logic. This is an attempt to rectify that.

• add a generic to_align_offset function to the geometry module to allow for aligning
• add an Align.NONE enum member
• change to_align_offset to return a Vector as every call site immediately constructed a vector anyway

Not sure if there is interest in merging such a pr. If so I can go ahead and make sure the documentation lines up with the changes and make any changes y'all deem appropriate.

Would close:

• Add Align.None #457

Supersedes:

• Add an "Align.NONE" enum member. #734

[codecov]

Codecov Report

Attention: Patch coverage is 88.09524% with 5 lines in your changes missing coverage. Please review.

Project coverage is 96.02%. Comparing base (d46b29d) to head (3b964ee).
Report is 1 commits behind head on dev.

Files with missing lines	Patch %	Lines
src/build123d/geometry.py	80.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #782      +/-   ##
==========================================
- Coverage   96.09%   96.02%   -0.07%     
==========================================
  Files          25       25              
  Lines        8474     8460      -14     
==========================================
- Hits         8143     8124      -19     
- Misses        331      336       +5     

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

[gumyr]

There should be some tests of Align.NONE (even though they might not increase coverage).

[gumyr]

I think this function would make more sense if the inputs were VectorLike then all of the math could be done with Vectors instead of looping over floats.

[erooke]

I'm not sure I follow how making them vectors helps us avoid the looping behavior in the general case. I have changed to VectorLike as the signature and the only place I could see to remove the looping behavior was in the trivial case where we have a single alignment variable.

[gumyr]

I'd like to introduce the following two TypeVars here that can be used on all of the objects (not necessarily in the PR):

Align2DType = TypeVar(
    "Align2DType", Union[Align, None], Tuple[Union[Align, None], Union[Align, None]]
)
Align3DType = TypeVar(
    "Align3DType",
    Union[Align, None],
    Tuple[Union[Align, None], Union[Align, None], Union[Align, None]],
)

This could be done later though.

[erooke]

I was under the impression that typevars were meant to signal generics. I have added these using type aliases as that feels more correct to me.

[gumyr]

Thanks for pointing this out, I think there are other examples of where TypeAlias should be used instead of TypeVar. This might help with the documentation as well as it seems as though ReadtheDocs will show the TypeAlias and not the full definition which will makes the docs more readable.

[erooke]

Whoops, responded to a comment mid it being edited 😅.

[gumyr]

I'm learning about TypeAlias :)

[gumyr]

align could also be None so I think should be typed as align: Sequence[Union[Align,None]].

[erooke]

That makes sense to me. I guess if we add the Align2DType and Align3DType it could be typed Union[Align2DType, Align3DType]

[erooke]

Made the requested changes, waiting to make sure the design aspects are ironed out before adding tests.

[gumyr]

Made the requested changes, waiting to make sure the design aspects are ironed out before adding tests.

I'm surprised there is no coverage for the existing Align tests - I would have guessed that there are tests that would have covered them already. Add a few tests for the Align options shouldn't be difficult though.