search

servo / euclid

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

Replace all `Into` impls with `From` impls #429

Closed hannobraun closed 1 year ago

hannobraun commented 4 years ago

As per Into's documentation, Into should not be implemented, if possible, and From should be implemented instead. This is more flexible, as there is a blanket impl that implements Into for all From.

hannobraun commented 4 years ago

Okay, looks like this won't work with Rust 1.31.0. I've converted this pull request into a draft, in case the minimum supported Rust version will be increased any time soon. Please let me know, if you want me to close it instead.

bors-servo commented 4 years ago

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

hannobraun commented 4 years ago

Rebased.

nical commented 4 years ago

I'm looking into opening up for breaking changes soon and make 0.21 release some time within the next few weeks in which we can incorporate a compiler version bump and this change.

bors-servo commented 4 years ago

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

hannobraun commented 4 years ago

Rebased.

nical commented 4 years ago

Looks like the language change that allows this was added in rustc 1.41.

Right now we are compatible with 1.31 and the current wave of breaking changes is a good opportunity to bump to a more recent version, however 1.41 is a bigge leap than I thought. For example that would make euclid incompatible with the rustc version packaged in debian (currently 1.34.2). I'd prefer a stronger motivation to trade compatibility, unless the missing blanket impls are causing grief.

hannobraun commented 4 years ago

I don't remember what my motivation for submitting this was, but I'm depending on upstream euclid. I must have been able to work around whatever problem I was having. So drastic measures are definitely unnecessary, as far as I'm concerned.

How do you want to proceed here? I don't mind keeping this open and rebased for now, but I can't guarantee that won't change in however many years it takes for Debian to get up to speed :-)

nical commented 4 years ago

We can keep this open here, or if having that one PR open causes frustation we can clause it and reopen later, I don't mind either way. No need to rebase it continuously in any case, it's a mechanical enough change that you or someone else can rebase/re-do it later when we finaly chose to depend on a more recent compiler version. Thanks in any case for your patience and understanding.

hannobraun commented 4 years ago

Sound good :+1:

bors-servo commented 1 year ago

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

ArtHome12 commented 1 year ago

@hannobraun Now the MSRV has been increased to 1.56 (Rust 2021). Do you have the ability to complete this request?

hannobraun commented 1 year ago

@ArtHome12: I've rebased the branch on latest master. cargo test passes, but I didn't do any testing beyond that. I no longer have any applications that use euclid.

nical commented 1 year ago

Thanks!

@bors-servo r+

bors-servo commented 1 year ago

:pushpin: Commit a5af28e has been approved by nical

bors-servo commented 1 year ago

:hourglass: Testing commit a5af28e206424f55b5b6dd017e4e0a30847de6a3 with merge 9d704545b77ed84bd76acf9ebddaa308d244e5b0...

bors-servo commented 1 year ago

:sunny: Test successful - checks-github Approved by: nical Pushing 9d704545b77ed84bd76acf9ebddaa308d244e5b0 to master...

bors-servo commented 1 year ago

:sunny: Test successful - checks-github Approved by: nical Pushing 9d704545b77ed84bd76acf9ebddaa308d244e5b0 to master...