Copilot commented on code in PR #120:
URL: https://github.com/apache/datasketches-rust/pull/120#discussion_r3213313777
##########
CONTRIBUTING.md:
##########
@@ -27,7 +27,7 @@ Rustup will read the `rust-toolchain.toml` file and set up
everything else autom
```shell
cargo version
-# cargo 1.85.0 (<hash> 2024-12-31)
+# cargo 1.86.0 (<hash> <date>)
Review Comment:
This example output now contains placeholders (`<hash> <date>`), which makes
it hard for contributors to compare against their actual `cargo version`
output. Consider either keeping a real sample (like before) or removing the
parenthesized portion entirely.
##########
xtask/src/main.rs:
##########
@@ -100,18 +146,35 @@ fn run_command(mut cmd: StdCommand) {
assert!(status.success(), "command failed: {status}");
}
-fn make_test_cmd(no_capture: bool, features: &[&str]) -> StdCommand {
+fn make_test_cmd<T: AsRef<str>>(no_capture: bool, features: &[T]) ->
StdCommand {
let mut cmd = find_command("cargo");
cmd.args(["test", "--workspace", "--no-default-features"]);
- if !features.is_empty() {
- cmd.args(["--features", features.join(",").as_str()]);
+ for feature in features {
+ cmd.args(["--features", feature.as_ref()]);
}
if no_capture {
cmd.args(["--", "--nocapture"]);
}
cmd
}
+fn make_check_cmd<T: AsRef<str>>(features: &[T]) -> StdCommand {
+ let mut cmd = find_command("cargo");
+ cmd.env("RUSTFLAGS", "-Dwarnings");
+ cmd.args([
+ "+nightly",
+ "check",
+ "--package",
+ "datasketches",
+ "--all-targets",
+ "--no-default-features",
+ ]);
Review Comment:
The new `x check` subcommand forces `cargo +nightly check`, but the CI test
job installs only `${{ matrix.rust-version }}` (MSRV/stable) and then runs
`cargo x check`. This will fail because the nightly toolchain isn’t installed,
and it also defeats the purpose of checking against MSRV. Prefer running `cargo
check` on the currently-selected toolchain (drop the `+nightly` override), or
ensure CI installs nightly in the test matrix if nightly is truly required.
##########
datasketches/Cargo.toml:
##########
@@ -34,6 +34,18 @@ keywords = ["sketch", "hyperloglog", "probabilistic"]
all-features = true
rustdoc-args = ["--cfg", "docsrs"]
+[features]
+default = []
+
+# Each sketch has its own feature, so that users can opt in to only the
sketches they need.
+bloom = []
+countmin = []
+cpc = []
+frequencies = []
+hll = []
+tdigest = []
+theta = []
+
Review Comment:
`datasketches` now defines per-sketch Cargo features, but `default = []`
contradicts the Issue #32 requirement (“By default, all sketches should be
turned on”). With the current defaults, users upgrading will lose all sketch
modules unless they opt-in via features (breaking change). Consider making
`default` include all sketch features (or add an `all` feature and set `default
= ["all"]`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
