whisperfish / rust-phonenumber

Library for parsing, formatting and validating international phone numbers.
Apache License 2.0
156 stars 55 forks source link

Bump dependencies, follow rustfmt and clippy #60

Closed ds-cbo closed 9 months ago

ds-cbo commented 11 months ago

Main reason for this PR is to bump strum to 0.25 so we can get rid of the syn 1.x dependency and move to syn 2.x

While working on it and running tests, cargo fmt and cargo clippy had several remarks, so I followed those as well

Most notably lazy_static! has been replaced with once_cell as preferred by clippy

codecov[bot] commented 11 months ago

Codecov Report

Attention: 74 lines in your changes are missing coverage. Please review.

Comparison is base (226b6fb) 71.39% compared to head (9a26c8d) 70.91%.

Files Patch % Lines
src/consts.rs 55.97% 70 Missing :warning:
src/carrier.rs 0.00% 1 Missing :warning:
src/error.rs 0.00% 1 Missing :warning:
src/extension.rs 0.00% 1 Missing :warning:
src/phone_number.rs 50.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #60 +/- ## ========================================== - Coverage 71.39% 70.91% -0.48% ========================================== Files 19 20 +1 Lines 1930 2022 +92 ========================================== + Hits 1378 1434 +56 - Misses 552 588 +36 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

djc commented 9 months ago

Friendly ping? Would be nice to get this released to avoid duplicate dependencies.

ds-cbo commented 9 months ago

@djc Is that a ping for me? I thought I processed all feedback and the ball is back at the maintainers now

rubdos commented 9 months ago

@djc Is that a ping for me? I thought I processed all feedback and the ball is back at the maintainers now

No it's to me, and I indeed forgot about this. Working on it!

rubdos commented 9 months ago

I took the liberty to push the dependency bumps on your branch here, in the way that I meant them. This version broadens the compatibility, without breaking any backwards compatibility:

As far as I can see, all dependencies are hereby up-to-date.

The only part that required Rust 1.62 was the derive(Default). I took the liberty to revert that commit, because it is rather trivial and keeps us 1.58-compatible. If we ever bump above 1.62, we can of course put it back.

I'm having a look at the test-dependencies now, such that we can have MSRV tests in CI too.

djc commented 9 months ago

I'm having a look at the test-dependencies now, such that we can have MSRV tests in CI too.

I recommend testing MSRV in CI with cargo check --lib --all-features, that is, avoiding tests and dev-dependencies for the purpose of MSRV testing.

rubdos commented 9 months ago

I recommend testing MSRV in CI with cargo check --lib --all-features, that is, avoiding tests and dev-dependencies for the purpose of MSRV testing.

Yes, looks like that's the way to go. I'm splitting this PR in two, one for the deps (#65) and one upcoming for the Lazy once_cell refactoring. The current state is a bit too much intertwined to decently review.

djc commented 9 months ago

FWIW, unless you put in the work to have CI jobs for each semver-incompatible branch (or have CI testing minimal-versions), I'm weary of >=0.10, <=0.12 style dependencies. I think this means Cargo will generally select 0.12.x in CI, so you're not actually testing whether 0.10 and 0.11 still work over time.

(Also, while I consider myself fairly conservative on MSRV issues, I think 1.6x is a pretty reasonable target at this point, especially for low x.)

rubdos commented 9 months ago

FWIW, unless you put in the work to have CI jobs for each semver-incompatible branch (or have CI testing minimal-versions), I'm weary of >=0.10, <=0.12 style dependencies. I think this means Cargo will generally select 0.12.x in CI, so you're not actually testing whether 0.10 and 0.11 still work over time.

I just tested, and apparently since the rust-version statements appeared in Cargo.toml, cargo is smart enough to check out the lower versions when necessary for lower rustc. Your point still stands though, there's no formal test around. Are you aware of any formal tests for these situations?

(Also, while I consider myself fairly conservative on MSRV issues, I think 1.6x is a pretty reasonable target at this point, especially for low x.)

I would still consider 1.61 at this point, since that's what SailfishOS will soon be on (if not 1.72). More out of personal "greed" or interest than anything else, for which I'm sorry.

rubdos commented 9 months ago

I'll close this in favor of #65 and #66.