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

Clippy fails because of chrono deprecation #950

Closed
Lorak-mmk opened this issue Mar 6, 2024 · 1 comment · Fixed by #951
Closed

Clippy fails because of chrono deprecation #950

Lorak-mmk opened this issue Mar 6, 2024 · 1 comment · Fixed by #951
Assignees

Comments

@Lorak-mmk
Copy link
Collaborator

New version of chrono was released a few hours ago.
It deprecated chrono::Duration::days, pointing to chrono::Duration::try_days instead.
This deprecation causes our CI to fail:
https://github.com/scylladb/scylla-rust-driver/actions/runs/8178066760/job/22361165132?pr=948#step:5:398

error: use of deprecated associated function `chrono::TimeDelta::days`: Use `TimeDelta::try_days` instead
   --> scylla-cql/src/frame/value.rs:492:59
    |
492 |         let duration_since_unix_epoch = chrono::Duration::days(days_since_unix_epoch);
    |                                                           ^^^^
    |
    = note: `-D deprecated` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(deprecated)]`

try_days was introduced in chrono 0.4.33 (or 0.4.32 - but docs.rs for this version don't build, so I didn't check).
In order to migrate, we need to bump our chrono dependency to 0.4.33 from the current 0.4.20 - so we need to require a very recent release. I'm a bit disappointed that they deprecate stuff so quickly after introducing replacement...

The alternative is to ignore this clippy error in this place, but I don't like this either.

This is the place where we use this method:

#[cfg(feature = "chrono")]
impl TryInto<NaiveDate> for CqlDate {
    type Error = ValueOverflow;

    fn try_into(self) -> Result<NaiveDate, Self::Error> {
        let days_since_unix_epoch = self.0 as i64 - (1 << 31);

        // date_days is u32 then converted to i64 then we subtract 2^31;
        // Max value is 2^31, min value is -2^31. Both values can safely fit in chrono::Duration, this call won't panic
        let duration_since_unix_epoch = chrono::Duration::days(days_since_unix_epoch);

        NaiveDate::from_yo_opt(1970, 1)
            .unwrap()
            .checked_add_signed(duration_since_unix_epoch)
            .ok_or(ValueOverflow)
    }
}

Given that this call can never fail, I'm wondering if it's better to unwrap() or return ValueOverflow.
I think I prefer unwrapping - panic will mean than there is an error in our code (or in the comment) which should be fixed.
Returnig ValueOverflow will not allow to distinguish between checked_add_signed fail and try_days fail.

@Lorak-mmk
Copy link
Collaborator Author

Oh, it's not the only deprecation. When running CI locally I see more:

error: use of deprecated associated function `chrono::TimeDelta::days`: Use `TimeDelta::try_days` instead
   --> scylla-cql/src/frame/value.rs:492:59
    |
492 |         let duration_since_unix_epoch = chrono::Duration::days(days_since_unix_epoch);
    |                                                           ^^^^
    |
    = note: `-D deprecated` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(deprecated)]`

error: use of deprecated associated function `chrono::NaiveDateTime::from_timestamp_opt`: use `DateTime::from_timestamp` instead
    --> scylla-cql/src/frame/response/result.rs:1586:41
     |
1586 |         let unix_epoch = NaiveDateTime::from_timestamp_opt(0, 0).unwrap().and_utc();
     |                                         ^^^^^^^^^^^^^^^^^^
     |
     = note: `-D deprecated` implied by `-D warnings`
     = help: to override `-D warnings` add `#[allow(deprecated)]`

error: use of deprecated associated function `chrono::TimeDelta::days`: Use `TimeDelta::try_days` instead
   --> scylla-cql/src/frame/value.rs:492:59
    |
492 |         let duration_since_unix_epoch = chrono::Duration::days(days_since_unix_epoch);
    |                                                           ^^^^

error: use of deprecated method `chrono::NaiveDateTime::timestamp_millis`: use `.and_utc().timestamp_millis()` instead
   --> scylla-cql/src/frame/value_tests.rs:498:32
    |
498 |             NaiveDateTime::MAX.timestamp_millis().to_be_bytes(),
    |                                ^^^^^^^^^^^^^^^^

error: use of deprecated method `chrono::NaiveDateTime::timestamp_millis`: use `.and_utc().timestamp_millis()` instead
   --> scylla-cql/src/frame/value_tests.rs:503:32
    |
503 |             NaiveDateTime::MIN.timestamp_millis().to_be_bytes(),
    |                                ^^^^^^^^^^^^^^^^

error: use of deprecated associated function `chrono::NaiveDateTime::from_timestamp_opt`: use `DateTime::from_timestamp` instead
   --> scylla-cql/src/frame/value_tests.rs:507:28
    |
507 |             NaiveDateTime::from_timestamp_opt(0, 0).unwrap(),
    |                            ^^^^^^^^^^^^^^^^^^

error: use of deprecated associated function `chrono::NaiveDateTime::from_timestamp_opt`: use `DateTime::from_timestamp` instead
   --> scylla-cql/src/frame/value_tests.rs:512:28
    |
512 |             NaiveDateTime::from_timestamp_opt(1, 0).unwrap(),
    |                            ^^^^^^^^^^^^^^^^^^

error: use of deprecated associated function `chrono::NaiveDateTime::from_timestamp_opt`: use `DateTime::from_timestamp` instead
   --> scylla-cql/src/frame/value_tests.rs:517:28
    |
517 |             NaiveDateTime::from_timestamp_opt(0, 1).unwrap(),
    |                            ^^^^^^^^^^^^^^^^^^

error: use of deprecated associated function `chrono::NaiveDateTime::from_timestamp_opt`: use `DateTime::from_timestamp` instead
   --> scylla-cql/src/frame/value_tests.rs:522:28
    |
522 |             NaiveDateTime::from_timestamp_opt(0, 1_000_000).unwrap(),
    |                            ^^^^^^^^^^^^^^^^^^

error: use of deprecated associated function `chrono::NaiveDateTime::from_timestamp_opt`: use `DateTime::from_timestamp` instead
   --> scylla-cql/src/frame/value_tests.rs:527:28
    |
527 |             NaiveDateTime::from_timestamp_opt(-2 * 24 * 60 * 60, 0).unwrap(),
    |                            ^^^^^^^^^^^^^^^^^^

chrono has quite a lot of deprecations and other changes lately

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant