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

Opt into y-down semantics by implementing a trait on the unit #361

Closed
wants to merge 1 commit into from

Conversation

@nical
Copy link
Collaborator

nical commented Jul 10, 2019

Alternative to #353.

Instead of using a global feature flag, only implement methods with names that depend on the meaning of the y-axis orientation if the unit implements the YDown trait.
The main difference is that the choice is made on a per-unit basis instead of a per-dependency basis.

The upsides are:

  • One can mix y-up and y-down units in the same crate without having some of them expose a bogus set of method names.
  • no need for an extra feature flag.

On the other hand:

  • The semantics of the y-axis are specified where the unit is declared, so it is no longer possible for a crate that is agnostic and one that wants y-down methods to interoperate with the same type. In other words, code that is agnostic to the y axis orientation, by exposing units that don't opt into YDown prevent all other users of these types from having y-down semantics, which is not great (but not the end of the world either).
  • By extension, the default UnknownUnit types cannot have y-down methods (or alternatively, they would be forced to always have them, but IMO that would be the wrong default).

Either way there is no intent to add y-up specific methods.


This change is Reviewable

@nical
Copy link
Collaborator Author

nical commented Jul 10, 2019

@kvark @Manishearth @SimonSapin @asajeffrey

Which do you prefer between this approach and the one in #353 (or some other alternative)?

@kvark
Copy link
Member

kvark commented Jul 10, 2019

I like this much more than #353 , I almost very much like this :)
There is still an appeal in making euclid completely agnostic, as suggested in #353 (comment) though, for the reason you list here (interop with other libraries that want to be agnostic).

@nical
Copy link
Collaborator Author

nical commented Jul 10, 2019

Indeed I should have mentioned this other possibility in my initial comment.

I am not against making euclid completely agnostic (removing all y-down specific names), but since it can be a pretty important change for code that relies a lot on these conventions (I suspect servo layout would be more impacted than webrender for example), I want to make the sure interested parties are OK with something like that.

What would the declaration of SideOffsets2D be like in a world where top, left, etc. can't be used?

@Manishearth
Copy link
Member

Manishearth commented Jul 10, 2019

Eh, I like this approach in principle, but I just spent way too much time dealing with a typed API that I needed to mold into the kinds of types that would work -- because a lot of what was compile time in the API was runtime for me. Typed units are nice, but this forces people to use them and I don't think they work out in all situations. UnknownUnit needs to implement the trait, but if we do that then you lose the opt in.

@nical
Copy link
Collaborator Author

nical commented Jul 10, 2019

UnknownUnit needs to implement the trait, but if we do that then you lose the opt in.

If UnknownUnit needs to implement the trait, then I am not enthused by this approach because:

  • The people who complain about y-down semantics tend to only use the default types with UnknownUnit.
  • I find that additive changes are easier to reason about (I mean that by adding a unit you lose some methods compared to the default, as opposed to starting with the same methods).
  • More generally I think that it makes more sense for the default type to be agnostic and let people opt into their specific needs, than have the default type have the specifics, and opt into being agnostic by adding a unit.

So in that case I'd rather make it a feature flag.

This indeed forces people who do want y-down semantics to use units, is your concern related to servo code or is it in general? Since servo crates use units in a lot of places I expected this wouldn't be an issue there.

What is your take on getting rid of all y-down specific methods as @kvark proposed?

@Manishearth
Copy link
Member

Manishearth commented Jul 10, 2019

This indeed forces people who do want y-down semantics to use units, is your concern related to servo code or is it in general?

General, not a servo thing. Typed units are great but you are often forced to cast to work around them when they don't quite match up to what you need, and it can get ugly.

Feature flag would be totally fine.

I don't have an opinion on getting rid of them, I haven't dealt with the servo code using it as much.

@bors-servo
Copy link
Contributor

bors-servo commented Jul 15, 2019

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

@nical nical closed this Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.