whisperfish / rust-phonenumber

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

Bump dependencies #65

Closed rubdos closed 10 months ago

rubdos commented 10 months ago

Follow-up from #60, such that #60 can focus on the lazy_static -> OnceCell transition.

codecov[bot] commented 10 months ago

Codecov Report

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

Comparison is base (226b6fb) 71.39% compared to head (d70a9d4) 71.18%.

Files Patch % Lines
src/error.rs 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #65 +/- ## ========================================== - Coverage 71.39% 71.18% -0.22% ========================================== Files 19 19 Lines 1930 1933 +3 ========================================== - Hits 1378 1376 -2 - Misses 552 557 +5 ```

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

rubdos commented 10 months ago

@djc, let's take the dependency and CI discussion here, this is a more isolated pull request.

djc commented 10 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?

I basically never do range dependencies like this -- the complexity isn't worth it to me. The canonical way to test this stuff would be with -Z minimal-versions but even then you'd only be testing 0.10, not 0.11, so there's no good guarantees.

(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.

1.61 makes sense to me, that's what we've been using for rustls.

rubdos commented 10 months ago

I basically never do range dependencies like this -- the complexity isn't worth it to me. The canonical way to test this stuff would be with -Z minimal-versions but even then you'd only be testing 0.10, not 0.11, so there's no good guarantees.

Interesting, thanks for sharing. To me, testing MSRV and latest stable is good enough for now. I suppose the real fix would be to have the upstream deps release a 1.x.

1.61 makes sense to me, that's what we've been using for rustls.

I'll consider 1.61 next. There is currently nothing really pushing us there as far as I can tell. Only the derive(Default), but that is 1.62 material anyway.

Thanks for your valuable input! I have implemented your suggestion for CI :)

rubdos commented 10 months ago

@ds-cbo / @djc, does either of you want to do a review on this, before I merge? Gabriel is currently ill, so I'd rather spare him a bit :-)