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

Rename chrono, time and secret features #939

Merged
merged 6 commits into from
Jun 19, 2024

Conversation

muzarski
Copy link
Contributor

@muzarski muzarski commented Feb 22, 2024

Ref: #771

Motivation

To follow the same naming conventions as for other features, we want to add a crate version numbers to the remaining dependencies hidden behind crate features: chrono, time and secrecy.

Changes

Dependecies versions

I bumped secrecy crate version from 0.7.0 to 0.8.0 which is the most recent version.

Feature names

Renamed following features (and dependencies):

  • feature chrono => feature chrono-04, dependency chrono => dependency chrono_04 (scylla-cql)
  • feature time => feature time-03, dependency time => dependency time_03 (scylla-cql)
  • feature secret => feature secrecy-08, dependency secrecy => dependency secrecy_08

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

@muzarski muzarski self-assigned this Feb 22, 2024
scylla/Cargo.toml Outdated Show resolved Hide resolved
scylla-cql/Cargo.toml Outdated Show resolved Hide resolved
scylla-cql/Cargo.toml Outdated Show resolved Hide resolved
@muzarski muzarski force-pushed the add_versions_to_feature_names branch from c339175 to f4733ca Compare February 28, 2024 13:20
@muzarski
Copy link
Contributor Author

v2: reverted the changes that dropped the minimum req version of time and chrono dependencies

@@ -10,7 +10,7 @@ futures = "0.3.6"
openssl = "0.10.32"
rustyline = "9"
rustyline-derive = "0.6"
scylla = {path = "../scylla", features = ["ssl", "cloud", "chrono", "time", "num-bigint-03", "num-bigint-04", "bigdecimal-04"]}
scylla = {path = "../scylla", features = ["ssl", "cloud", "chrono-04", "time-03", "num-bigint-03", "num-bigint-04", "bigdecimal-04"]}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this PR, but: why do we keep here both num-bigint-03 and num-bigint-04 at the same time?

Copy link
Contributor Author

@muzarski muzarski Mar 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not like both of these features cannot be used at the same time. The user might (for some weird reason) want to support (de)serialization for both versions of num_bigint::BigInt.

There is other problem, though - BigInt is not used in the examples at all... This means that we do not make use of any of these features anyway. I'm not sure why I added these features to examples. Probably for some reason related to vscode rust-analyzer. As I remember, the vscode' analyzer compiles the driver code with the features used in the examples/ by default (it is helpful to detect errors under #[cfg(feature = ...)]). However, this can be configured in the workspace settings.

I think we could remove unused features from examples, but is it really necessary?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we really want to have all features enabled in examples, I'd suggest using "full_serialization" feature instead of listing them all manually.

scylla-cql/Cargo.toml Show resolved Hide resolved
Copy link
Collaborator

@Lorak-mmk Lorak-mmk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd really like to see support for 2 major versions of each lib (as is done with num-bigint) - that way we would know that we are actually able to support more than one

scylla-cql/src/frame/response/result.rs Outdated Show resolved Hide resolved
scylla-cql/src/frame/response/result.rs Outdated Show resolved Hide resolved
scylla-cql/src/frame/response/result.rs Outdated Show resolved Hide resolved
@wprzytula wprzytula added waiting-on-author Waiting for a response from issue/PR author API-stability Part of the effort to stabilize the API and removed waiting-on-author Waiting for a response from issue/PR author labels Mar 26, 2024
@muzarski muzarski force-pushed the add_versions_to_feature_names branch from f4733ca to 88607ab Compare April 24, 2024 14:56
@muzarski
Copy link
Contributor Author

v2.1: rebased on top of main - to check if CI passes

@muzarski muzarski force-pushed the add_versions_to_feature_names branch 2 times, most recently from 1abe8d5 to cde43f6 Compare April 25, 2024 14:09
@github-actions github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Apr 25, 2024
Copy link

github-actions bot commented Apr 25, 2024

cargo semver-checks detected some API incompatibilities in this PR.
Checked commit: cc705ad

See the following report for details:

cargo semver-checks output
./scripts/semver-checks.sh --baseline-rev ab9a80e2f04c9af50b3077e08cf77d4976c2f8af
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev ab9a80e2f04c9af50b3077e08cf77d4976c2f8af
     Cloning ab9a80e2f04c9af50b3077e08cf77d4976c2f8af
     Parsing scylla v0.13.0 (current)
      Parsed [  24.219s] (current)
     Parsing scylla v0.13.0 (baseline)
      Parsed [  23.581s] (baseline)
    Checking scylla v0.13.0 -> v0.13.0 (no change)
     Checked [   0.531s] 72 checks; 72 passed, 0 unnecessary
    Finished [  48.393s] scylla
     Parsing scylla-cql v0.2.0 (current)
      Parsed [  10.439s] (current)
     Parsing scylla-cql v0.2.0 (baseline)
      Parsed [  10.553s] (baseline)
    Checking scylla-cql v0.2.0 -> v0.2.0 (no change)
     Checked [   0.325s] 72 checks; 71 passed, 1 failed, 0 unnecessary

--- failure inherent_method_missing: pub method removed or renamed ---

Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.31.0/src/lints/inherent_method_missing.ron

Failed in:
  CqlValue::as_naive_date, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-ab9a80e2f04c9af50b3077e08cf77d4976c2f8af/8ba87b8bb0d05226b6ec5b7b8a81cd584f31b3f2/scylla-cql/src/frame/response/result.rs:191
  CqlValue::as_date, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-ab9a80e2f04c9af50b3077e08cf77d4976c2f8af/8ba87b8bb0d05226b6ec5b7b8a81cd584f31b3f2/scylla-cql/src/frame/response/result.rs:196
  CqlValue::as_datetime, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-ab9a80e2f04c9af50b3077e08cf77d4976c2f8af/8ba87b8bb0d05226b6ec5b7b8a81cd584f31b3f2/scylla-cql/src/frame/response/result.rs:208
  CqlValue::as_offset_date_time, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-ab9a80e2f04c9af50b3077e08cf77d4976c2f8af/8ba87b8bb0d05226b6ec5b7b8a81cd584f31b3f2/scylla-cql/src/frame/response/result.rs:213
  CqlValue::as_naive_time, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-ab9a80e2f04c9af50b3077e08cf77d4976c2f8af/8ba87b8bb0d05226b6ec5b7b8a81cd584f31b3f2/scylla-cql/src/frame/response/result.rs:225
  CqlValue::as_time, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-ab9a80e2f04c9af50b3077e08cf77d4976c2f8af/8ba87b8bb0d05226b6ec5b7b8a81cd584f31b3f2/scylla-cql/src/frame/response/result.rs:230
     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  21.369s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

@muzarski muzarski force-pushed the add_versions_to_feature_names branch from cde43f6 to 7a0d3c9 Compare April 25, 2024 14:57
@muzarski muzarski force-pushed the add_versions_to_feature_names branch 6 times, most recently from 5041458 to 36dd1ad Compare May 14, 2024 06:50
@muzarski
Copy link
Contributor Author

I'd really like to see support for 2 major versions of each lib (as is done with num-bigint) - that way we would know that we are actually able to support more than one

With @Lorak-mmk, we had a discussion offline and decided not to introduce support for older versions of chrono and time crates. The commits that introduce support for time 0.3 and chrono 0.4 are basically a showcase that the support for future versions of these crates can be easily introduced. These commits should be dropped before merge.

@muzarski muzarski removed the waiting-on-author Waiting for a response from issue/PR author label May 15, 2024
@wprzytula wprzytula added this to the 0.14.0 milestone May 21, 2024
@muzarski muzarski force-pushed the add_versions_to_feature_names branch from 36dd1ad to 04fea72 Compare May 22, 2024 12:14
@muzarski
Copy link
Contributor Author

v2.2: rebased on top of main

@Lorak-mmk Lorak-mmk self-assigned this Jun 1, 2024
@wprzytula
Copy link
Collaborator

@muzarski Please remove mock commits, then I'll review the final version.

@muzarski muzarski force-pushed the add_versions_to_feature_names branch from 04fea72 to 9c35df8 Compare June 12, 2024 12:53
@wprzytula wprzytula removed their request for review June 12, 2024 15:26
@muzarski muzarski force-pushed the add_versions_to_feature_names branch from 9c35df8 to 3fba8fd Compare June 13, 2024 09:51
@muzarski
Copy link
Contributor Author

Rebased on main. I'll wait for CI to pass and then drop the mock commits.

@muzarski muzarski force-pushed the add_versions_to_feature_names branch from 3fba8fd to a850ee4 Compare June 13, 2024 10:04
@muzarski
Copy link
Contributor Author

v3:

  • adjusted job names in CI
  • dropped mock commits

@muzarski muzarski requested a review from wprzytula June 13, 2024 10:07
scylla-cql/src/frame/response/cql_to_rust.rs Outdated Show resolved Hide resolved
scylla-cql/src/frame/response/cql_to_rust.rs Outdated Show resolved Hide resolved
scylla-cql/src/frame/response/cql_to_rust.rs Outdated Show resolved Hide resolved
scylla-cql/src/frame/response/cql_to_rust.rs Outdated Show resolved Hide resolved
scylla-cql/src/frame/response/result.rs Outdated Show resolved Hide resolved
scylla-cql/src/types/serialize/value.rs Outdated Show resolved Hide resolved
@wprzytula
Copy link
Collaborator

Is the new deserialization code that uses time, chrono and secrecy deps adjusted as well?

@muzarski muzarski force-pushed the add_versions_to_feature_names branch from a850ee4 to 3e3c984 Compare June 13, 2024 13:00
@muzarski
Copy link
Contributor Author

Is the new deserialization code that uses time, chrono and secrecy deps adjusted as well?

Good point.

Since some other version of time_03 might be supported in the future,
we cannot have multiple methods such as `as_date()` defined.

I don't think it's good idea to expose version specific methods
(e.g. `as_date_03()`) to the users. This is why we make them
test utilities only.
Made `CqlValue`-to-chrono types conversion methods a private
utility methods used only for tests.
@muzarski muzarski force-pushed the add_versions_to_feature_names branch from 3e3c984 to cc705ad Compare June 13, 2024 13:09
@muzarski
Copy link
Contributor Author

v3.1:

  • addressed minor issues from review comments
  • adjusted deserialization module to make use of proper feature flags

@muzarski muzarski requested a review from wprzytula June 13, 2024 14:50
@wprzytula wprzytula self-assigned this Jun 19, 2024
@muzarski
Copy link
Contributor Author

@Lorak-mmk @wprzytula It has 2 approves. Can we finally merge this?

@wprzytula wprzytula merged commit 0f73b6a into scylladb:main Jun 19, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API-stability Part of the effort to stabilize the API semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants