search

bschwind / opencascade-rs

Rust bindings to the OpenCascade CAD Kernel
GNU Lesser General Public License v2.1
118 stars 22 forks source link

Alternate angle implementation with two types #92

Closed PabloMansanet closed 1 year ago

PabloMansanet commented 1 year ago

Just responding to @strohel's nerdsnipe at https://github.com/bschwind/opencascade-rs/pull/85#issuecomment-1636779978 :)

This kind of API is usually my preference, though it's not a strong preference. It is true that is more verbose in many places.

the 45.degrees() syntax was easy to implement, and there's the benefit that a user that never wants to care about either unit can just ignore it and work entirely on Degrees or Radians without ever having to mess with the enum.

I'm not happy about the into().into() chains where a Into<Radians> implementer has to then be converted to f64, but I wasn't really sure that into().value() would have been much better.

If there's one thing to take away from this PR, I think that this in particular is a bit nicer than implementing for f64 and u64 separately:

impl<T: Into<f64> + Copy> ToAngle for T

Since this ToAngle trait forces the caller to call degrees() or radians() explicitly, I think it's a net positive to have a blanket impl so people can make angles from u32 and u8s or whatever they like.

bschwind commented 1 year ago

I'm biased, but I'm not seeing the concrete ways this improves upon the existing enum approach.

Also requiring impl Into<Radians> and x.into().into() feels more verbose, is more opaque (why do I need .into() twice?), and likely generates more code with monomorphization.

I agree the impl<T: Into<f64> + Copy> ToAngle for T blanket impl is nice.

I can be persuaded though - what tangible, concrete benefits does this API add over the existing solution?

strohel commented 1 year ago

Yeah, thinking about this more, I'm no longer sure which is better. #85 is shorter and simpler in some sense, so we may as well go with it if we don't want to think too much about it?

Sorry @PabloMansanet for nerd-sniping you into something that may not be used in the end.

bschwind commented 1 year ago

Closing in favor of #85