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

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

Merged
merged 1 commit into from Mar 26, 2020

Conversation

@Mingun
Copy link
Contributor

Mingun commented Mar 13, 2020

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
Copy link
Collaborator

nical commented Mar 13, 2020

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

Mingun commented Mar 13, 2020

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
Copy link
Collaborator

nical commented Mar 13, 2020

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

bors-servo commented Mar 25, 2020

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

@Mingun Mingun force-pushed the Mingun:provide-approx_eq branch from 8ae1c9e to 543fc3c Mar 25, 2020
@Mingun
Copy link
Contributor Author

Mingun commented Mar 25, 2020

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

Copy link
Collaborator

nical left a comment

Please don't reformat the code. Looks good otherwise.

@Mingun Mingun force-pushed the Mingun:provide-approx_eq branch from 543fc3c to 7c8795c Mar 25, 2020
@Mingun
Copy link
Contributor Author

Mingun commented Mar 25, 2020

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
Copy link
Collaborator

nical commented Mar 26, 2020

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

bors-servo commented Mar 26, 2020

📌 Commit 7c8795c has been approved by nical

@bors-servo
Copy link
Contributor

bors-servo commented Mar 26, 2020

Testing commit 7c8795c with merge 59d6f6a...

@bors-servo
Copy link
Contributor

bors-servo commented Mar 26, 2020

☀️ Test successful - checks-travis
Approved by: nical
Pushing 59d6f6a to master...

@bors-servo bors-servo merged commit 59d6f6a into servo:master Mar 26, 2020
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
@Mingun Mingun deleted the Mingun:provide-approx_eq branch 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

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