Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add getters providing strongly typed Length values #461

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nical
Copy link
Contributor

@nical nical commented Jul 28, 2020

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
Copy link
Contributor Author

nical commented Jul 28, 2020

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
Copy link
Contributor Author

nical commented Jul 28, 2020

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 nical requested a review from kvark July 28, 2020 09:32
@kvark
Copy link
Member

kvark commented Jul 28, 2020

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

@bors-servo
Copy link
Contributor

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

@ogoffart
Copy link

ogoffart commented Apr 21, 2021

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.

@nical
Copy link
Contributor Author

nical commented Apr 22, 2021

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?

@kvark
Copy link
Member

kvark commented Apr 22, 2021

Sure

@mrobinson
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should Vector2D<T,U>::length() return Length<T,U>
5 participants