search

servo / euclid

Geometry primitives (basic linear algebra) for Rust
Other
458 stars 102 forks source link

Provide `approx_eq` and implement `ApproxEq` for Transform2D and Transform3D #409

Closed Mingun closed 4 years ago

Mingun commented 4 years ago

Other geometry primitives implements that trait, so for consistency I implement it for Transform objects too.

This is small breaking change in Transform2D and Transform3D -- now you must import approxeq::ApproxEq if you want to compare transforms approximately.

nical commented 4 years ago

Thanks. Instead of removing the non trait implementation of approx_eq, please keep them and have the trait implementations call them. This way you don't need to import the traits which I prefer, and it doesn't cause a breaking change. You can add #![deny(unconditional_recursion)] to the crate root to make sure a type won't accidentally cause the trait impl of approx_eq to call itslef instead of the apporx_eq method.

More generally we avoid making semver breaking changes in euclid unless there is a strong motivation.

Mingun commented 4 years ago

It's going to be a bit inconsistent because other types just implement the trait. It is likely that in real code it will already be imported. From this point of view it is soft breaking change. And anyway right now I preparing some PRs with other soft breaking changes, but their improve consistency with rust ecosystem, so I'll wait while I finish it and then maybe you change your opinion.

nical commented 4 years ago

The inconsistency can be addressed backward-compatbly the other way around, having other types implement approx_eq as a method in addition to exposing it through the trait. While there is a bit of a grey area with old compiler versions this is a breaking change. For example webrender stops compiling if this changes without a breaking semver bump.

And anyway right now I preparing some PRs with other soft breaking changes, but their improve consistency with rust ecosystem

I am sorry but these will have to wait potentially for a long time. Aesthetic concerns are not strong enough reasons to make semver breaking changes to euclid. If you want changes to land soon, I would suggest focusing on non breaking ones. It's still OK for you to propose some breaking changes as long as you don't mind waiting.

bors-servo commented 4 years ago

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

Mingun commented 4 years ago

All review comments addressed. Now Transforms implement trait additionally to their methods

Mingun commented 4 years ago

Please don't reformat the code.

This is not a whim, but a means of avoiding typos when writing code in repetitive data. In addition, such a style is already used elsewhere, so I do not understand why it should not be improved in the remaining ones. But if you're against... I reverted format changes

nical commented 4 years ago

Thanks for reverting the reformatting. Whether this or that style is an improvement over another is a very subjective thing. When it comes to subjective topics, its best to leave let the maintainer of the code decide. It's OK to argue about it as long as you don't pour your heart and soul into it, it's not a good use of your time and energy.

I'll do a formatting pass over the code and address inconsistencies to settle the matter once we are done with incoming pull requests to avoid generating code conflicts.

@bors-servo r+

bors-servo commented 4 years ago

:pushpin: Commit 7c8795c has been approved by nical

bors-servo commented 4 years ago

:hourglass: Testing commit 7c8795c0a602e1a79410758e542e79b502fc3653 with merge 59d6f6acf6f595f9521fdf735208bed6fbb9005c...

bors-servo commented 4 years ago

:sunny: Test successful - checks-travis Approved by: nical Pushing 59d6f6acf6f595f9521fdf735208bed6fbb9005c to master...