Add num_traits::Euclid

[ArtHome12]

I would like to suggest adding support for Euclidean division and, especially, remainder. It is necessary because in Rust the % operator does not produce the result that is intuitively expected from it - https://stackoverflow.com/questions/31210357/is-there-a-modulus-not-remainder-function-operation Perhaps that is why % is not implemented along with other operations from std::ops

This change will simplify the users code as shown below.

      let t = Point2D...
      let s = Size2D...

      // Old code:
      // let x = ((t.x % s.width) + s.width) % s.width;
      // let y = ((t.y % s.height) + s.height) % s.height;
      // Point2D::new(x, y)

      // New code with less place for mistakes:
      t.rem_euclid(&s)

Arguments are declared with & because num_traits::ops::euclid::Euclid is specified as such. https://docs.rs/num-traits/0.2.15/num_traits/ops/euclid/trait.Euclid.html

If this PR is accepted, I will prepare a similar one for the Vector.

[ArtHome12]

There is no one to review, the crate is dead?

[nical]

Sorry for the delay, the crate isn't dead but I'm very busy at the moment,

[nical]

@bors-servo r+

[bors-servo]

:pushpin: Commit 56df056 has been approved by nical

[bors-servo]

:hourglass: Testing commit 56df0560cf14051b5b5e5f8c06f76fbf7b644c6f with merge 5f5f30d54815554cc16d9bd597891699edd5ae83...

[bors-servo]

:broken_heart: Test failed - checks-github

[ArtHome12]

Sorry for the delay, the crate isn't dead but I'm very busy at the moment,

Glad to hear it! Take your time so as not to prejudice the main activities.

Problem due to using an old version with a not yet supported https://doc.rust-lang.org/std/f64/constant.EPSILON.html , but libm has been updated

info: The currently active `rustc` version is `rustc 1.67.1 (d5a82bbd2 2023-02-07)`
...
/home/runner/.cargo/bin/rustup toolchain install 1.34.0
info: latest update on 2019-04-11, rust version 1.34.0 (9[18]
  1.34.0-x86_64-unknown-linux-gnu installed - rustc 1.34.0 (91856ed52 [20]

[nical]

Hi, sorry to ask this but could you change the CI config in .github/workflows/main.yml so that we don't run with "--no-default-features --features libm" on rustc 1.34.0?

I'm no github expert but I think that you can achieve that by taking it out of the line

        features: ["", "--features serde", "--no-default-features --features libm"]

and adding just below "include:"

          - version: stable
            features: --no-default-features --features libm

That will make it run this config only on stable which is sufficient.

Also, unless you intend to follow this up with another PR, could you bump euclid to 0.22.8? That way I can publish your change as soon as this is merged.

[ArtHome12]

Ok, I'll do it this week if no one gets ahead of me. I want to also suggest division and multiplication by a scalar for the Scale https://github.com/servo/euclid/issues/499 before increase version, if there is no disagreement

[ArtHome12]

The features serde also does not correspond to the MSRV. Waiting for your decision - maybe raise the minimum version from 1.34 to 1.56 (requires syn) and return --features libm (requires 1.43) back into MSRV? https://doc.rust-lang.org/edition-guide/rust-2021/index.html

Linux (--features serde, 1.34.0)

Run cargo build --features serde
  cargo build --features serde
  shell: /usr/bin/bash -e {0}
    Updating crates.io index
 Downloading crates ...
  Downloaded serde v1.0.158
  Downloaded num-traits v0.2.15
  Downloaded autocfg v1.1.0
  Downloaded serde_derive v1.0.158
  Downloaded proc-macro2 v1.0.52
  Downloaded quote v1.0.26
  Downloaded syn v2.0.4
error: failed to parse manifest at `/home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/syn-2.0.4/Cargo.toml`

Caused by:
  failed to parse the `edition` key

Caused by:
  supported edition values are `2015` or `2018`, but `2021` is unknown
Error: Process completed with exit code 101.

[nical]

That's too bad. I wanted to play nice and avoid bumping the msrv in patch releases (and above all avoid breaking changes) but since multiple dependencies are doing it from under us I don't want us to spend too long swimming against the current. Let's do what you suggested in your latest comment. Also, thanks for your patience.

[ArtHome12]

Another problem with Github CI - Node 12 has been out of support since April 2022 and Github plan to migrate all actions to run on Node16 by Summer 2023 https://github.blog/changelog/2022-09-22-github-actions-all-actions-will-begin-running-on-node16-instead-of-node12/

To prevent this from happening all of a sudden, I suggest doing it now. We need to update actions/checkout@v2 (done) and actions-rs/toolchain@v1. The difficulty is that actions-rs unmaintained https://github.com/actions-rs/toolchain/issues/216

I can test a replacement like https://github.com/marketplace/actions/rustup-toolchain-install if you do not mind.

And what version will be after increase MSRV, still 22.8?

[nical]

Thanks, let's follow your suggestions.

And what version will be after increase MSRV, still 22.8?

Yes, let's make it 0.22.8. Breaking changes in euclid lead to a rather painful graph of crates to update in lockstep in the Firefox/Servo ecosystem. And technically you did not introduce a breaking change, we just acknowledge in our CI that newer patch versions of our dependencies changed their msrv without considering it breaking.

[ArtHome12]

v0.22.8

• Add num_traits::Euclid for point (rem_euclid() and div_euclid())
• Add Scale::clamp() for non-Ord T
• Bump MSRV from 1.34 to 1.56 (Rust2021) due to dependencies
• Replace Github CI actions-rs due to unmaintained. Bump actions/checkout

[nical]

@bors-servo r+

[bors-servo]

:pushpin: Commit 9e57337 has been approved by nical

[bors-servo]

:hourglass: Testing commit 9e573371881f5eb75aa1a967307b2abd0594f704 with merge 860d16009d349188713bbd408f417171d4483774...

[bors-servo]

:sunny: Test successful - checks-github Approved by: nical Pushing 860d16009d349188713bbd408f417171d4483774 to master...