euclid icon indicating copy to clipboard operation
euclid copied to clipboard

Add getters providing strongly typed Length values

Open nical opened this issue 3 years ago • 8 comments

Fixes #396 and #388.

This PR adds strongly typed Length getters with a simple naming scheme: anything prefixed with get_ returns a Length<T, U>, while unprefixed methods returning a scalar and direct scalar member accesses yield T directly.

The main motivations behind this approach are:

  • Simple and nicer naming scheme than what we tried before. We used to have things like v.x_typed() and it was just too awkward to use so we removed it. I think that get_ has less cognitive load and a long line of computation with lots of them is much easier to read than long lines with lots of *_typed suffixes.
  • It's easy to apply this naming scheme consistently even as we add new methods.
  • It doesn't get in the way of using euclid as a simple vector math crate without tagged units, which is pretty common.
  • Not a breaking change. Now is a good time to make breaking changes if they are worth the breakage, but not breaking downstream code unnecessarily is still a valuable thing.

nical avatar Jul 28 '20 08:07 nical

There are a few other getters to add but I'd like to discuss the naming scheme before doing the rest of the work.

nical avatar Jul 28 '20 08:07 nical

For reference, I tinkered in a separate branch with another naming scheme that avoids the get_ prefix:

  • length() and other methods that return distances/length-like semantic values return a Length instead of a T.
  • when there is a scalar member sich as x or width, there is a method with the same name (.x(), .width() etc.) returning a Length.

That scheme looks nice in principle, in practice it make the ergonomics of working with scalars quite a bit worse. We end up having to sprinkle .get() (Length::get(self) -> T) in many places as we use the length for computations where the result and/or intermediate steps don't have "length"-like semantics, for example the places where we divide by a vector's length, etc. Also it would break a lot of downstream code.

A subsequent commit implements Deref for Length in an attempt to make the sprinkling of get() a bit less ugly, but the cognitive load stays the same everywhere we need the scalars. I postulate that the most common case is to need the scalar directly and therefore getting lengths by default and having to unpack it in a lot of places is a wrong approach.

So as far as T vs Length is concerned I want two sets of methods wherever it makes sense, so that the scalar use case remains the simplest to work with, while still letting people opt into using Length wherever it is valuable to them.

A third option is to make the API as nice to use as possible at the expense of consistency, and keep the nicest names wherever we can. We would skip the prefix for getters where the value can be accessed directly as a scalar member (for example size.width() returns a Length), while methods that compute a scalar would have an equivalent with a prefix (for example vector.length() would continue returning a scalar and we'd have vector.get_length() or other name returning a Length).

nical avatar Jul 28 '20 09:07 nical

Ok, let's defer this until after the release, since it's not breaking.

kvark avatar Jul 28 '20 14:07 kvark

:umbrella: The latest upstream changes (presumably #476) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo avatar Feb 24 '21 16:02 bors-servo

Any update? I was trying to do some type safe unit handling, and this would be quite welcome.

Edit: but actually it would also be nice to have it in more even types like all the accessors of Rect that currently return T directly.

ogoffart avatar Apr 21 '21 12:04 ogoffart

I'm still keen on adding strongly typed variants of methods that return scalars where it makes sense using the get_* prefix. @kvark are you on board with it?

nical avatar Apr 22 '21 09:04 nical

Sure

kvark avatar Apr 22 '21 14:04 kvark

We're interested in this feature in Servo, so if there is no opposition, we'd really like to land this. We're happy to continue the discussion about the name of these APIs, but otherwise these seem really useful.

mrobinson avatar Sep 12 '23 09:09 mrobinson