From 58b8d23e68c3267e1e59ff3f4661efbf250fcf14 Mon Sep 17 00:00:00 2001 From: Jan-Peter von Hunnius Date: Sat, 15 Aug 2026 19:29:51 +0200 Subject: [PATCH] Address full-codebase review: logging boundary, clone elimination, robustness Library no longer prints to stderr: - Replace every eprintln! with log::warn! (lib: user_data, http; bin: parsing, output, commands); initialize env_logger in main with a warn-level default so output is unchanged unless RUST_LOG is set - Warn on missing/non-numeric "t" time fields instead of silently coercing them to 0.0; suppress the per-car NotFound warning (most cars legitimately have no timing file), keep warnings for real I/O errors Ownership and clone cleanup: - CarCounts no longer duplicates the car database: drop the car_list field and build car_map by consuming car_list (each Car stored once); iterate car_map.values() where the list was previously used - run_analysis takes MyCarsData by value: owned moves into the parse Arc, owned_configs/copy_count move into CarCounts; only the three commands that need owned/wishlist afterwards retain their own copy - is_final_build checks the digits directly (no per-call String allocation) and treats malformed configs as non-final so they surface in improve.csv; owned_final_configs becomes a borrowed rid-to-configs map so the target-exclusion check allocates nothing - Normalize sum arithmetic to u8 in normalize_to_final_build Docs and structure: - Document the first-seen-wins tie-breaking in compute_ideal_wishlist and the non-final/non-best coverage gap in write_trash_csv - Extract compare_by_rq_config_brand_name shared by compare_records and the remove.csv sort; drop the unused _output_dir parameter from run_track; add a retry_delay helper de-duplicating the three HTTP backoff sites; rename the shadowed `entry` binding to config_data --- Cargo.lock | 131 ++++++++++++++++++++++++++++++++++++--- Cargo.toml | 2 + src/commands/all.rs | 15 ++--- src/commands/best.rs | 2 +- src/commands/details.rs | 2 +- src/commands/improve.rs | 2 +- src/commands/mod.rs | 25 ++++---- src/commands/track.rs | 7 +-- src/commands/trash.rs | 5 +- src/commands/wishlist.rs | 13 ++-- src/context.rs | 12 ++-- src/http.rs | 32 +++++----- src/output/mod.rs | 78 ++++++++++++++--------- src/parsing/mod.rs | 28 ++++++--- src/records.rs | 4 +- src/types.rs | 15 +++-- src/user_data.rs | 2 +- 17 files changed, 255 insertions(+), 120 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6574a9e..03e902a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -110,6 +110,12 @@ version = "0.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5e764a1d40d510daf35e07be9eb06e75770908c27d411ee6c92109c9840eaaf7" +[[package]] +name = "bitflags" +version = "1.3.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bef38d45163c2f1dde094a7dfd33ccf595c92905c8f8f4fdc18d06fb1037718a" + [[package]] name = "bitflags" version = "2.11.1" @@ -245,6 +251,37 @@ dependencies = [ "parking_lot_core", ] +[[package]] +name = "defmt" +version = "1.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e2953bfe4f93bbd20cc71198842756f77d161884c99ebbabc41d80231ded88d1" +dependencies = [ + "bitflags 1.3.2", + "defmt-macros", +] + +[[package]] +name = "defmt-macros" +version = "1.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bad9c72e7ca2137e0dc3813245a0d282fd6daad32fd800af018306a9169b5fe8" +dependencies = [ + "defmt-parser", + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "defmt-parser" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "10d60334b3b2e7c9d91ef8150abfb6fa4c1c39ebbcf4a81c2e346aad939fee3e" +dependencies = [ + "thiserror", +] + [[package]] name = "displaydoc" version = "0.2.5" @@ -265,6 +302,29 @@ dependencies = [ "cfg-if", ] +[[package]] +name = "env_filter" +version = "2.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "900d271a03799a1ee8d1ca9b19893b48ca674a9284fefcfb85f05e74ed314217" +dependencies = [ + "log", + "regex", +] + +[[package]] +name = "env_logger" +version = "0.11.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "de671bd27a75a797dc9ae289ba1e77276e75e2026408aab65185384e2d5cd3f6" +dependencies = [ + "anstream", + "anstyle", + "env_filter", + "jiff", + "log", +] + [[package]] name = "equivalent" version = "1.0.2" @@ -761,6 +821,42 @@ version = "1.0.18" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8f42a60cbdf9a97f5d2305f08a87dc4e09308d1276d28c869c684d7777685682" +[[package]] +name = "jiff" +version = "0.2.35" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "668b7183bd07af9a4885f5c35b0cc5c83c4607a913c16b7e17291832910d2dcc" +dependencies = [ + "defmt", + "jiff-core", + "jiff-static", + "log", + "portable-atomic", + "portable-atomic-util", + "serde_core", +] + +[[package]] +name = "jiff-core" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7feca88439efe53da3754500c1851dedf3cb36c524dd5cf8225cc0794de95d09" +dependencies = [ + "defmt", +] + +[[package]] +name = "jiff-static" +version = "0.2.35" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3a69dcb3a21cfb32ce1cd056169337ca284af0766dd766e7878819b251a49204" +dependencies = [ + "jiff-core", + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "js-sys" version = "0.3.97" @@ -904,7 +1000,7 @@ version = "0.10.79" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "bf0b434746ee2832f4f0baf10137e1cabb18cbe6912c69e2e33263c45250f542" dependencies = [ - "bitflags", + "bitflags 2.11.1", "cfg-if", "foreign-types", "libc", @@ -982,6 +1078,21 @@ version = "0.3.33" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "19f132c84eca552bf34cab8ec81f1c1dcc229b811638f9d283dceabe58c5569e" +[[package]] +name = "portable-atomic" +version = "1.15.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "05c8b63e8d9609db387f0324918f81d68fe27748f084ef092fb35954d0539a85" + +[[package]] +name = "portable-atomic-util" +version = "0.2.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c2a106d1259c23fac8e543272398ae0e3c0b8d33c88ed73d0cc71b0f1d902618" +dependencies = [ + "portable-atomic", +] + [[package]] name = "potential_utf" version = "0.1.5" @@ -1027,7 +1138,7 @@ checksum = "4b45fcc2344c680f5025fe57779faef368840d0bd1f42f216291f0dc4ace4744" dependencies = [ "bit-set", "bit-vec", - "bitflags", + "bitflags 2.11.1", "num-traits", "rand", "rand_chacha", @@ -1109,7 +1220,7 @@ version = "0.5.18" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ed2bf2547551a7053d6fdfafda3f938979645c44812fbfcda098faae3f1a362d" dependencies = [ - "bitflags", + "bitflags 2.11.1", ] [[package]] @@ -1201,7 +1312,7 @@ version = "1.1.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b6fe4565b9518b83ef4f91bb47ce29620ca828bd32cb7e408f0062e9930ba190" dependencies = [ - "bitflags", + "bitflags 2.11.1", "errno", "libc", "linux-raw-sys", @@ -1286,7 +1397,7 @@ version = "3.7.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b7f4bc775c73d9a02cde8bf7b2ec4c9d12743edf609006c7facc23998404cd1d" dependencies = [ - "bitflags", + "bitflags 2.11.1", "core-foundation 0.10.1", "core-foundation-sys", "libc", @@ -1453,7 +1564,7 @@ version = "0.7.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a13f3d0daba03132c0aa9767f98351b3488edc2c100cda2d2ec2b04f3d8d3c8b" dependencies = [ - "bitflags", + "bitflags 2.11.1", "core-foundation 0.9.4", "system-configuration-sys", ] @@ -1577,7 +1688,9 @@ version = "0.3.0" dependencies = [ "clap", "dashmap", + "env_logger", "futures", + "log", "mockito", "proptest", "reqwest", @@ -1610,7 +1723,7 @@ version = "0.6.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a28f0d049ccfaa566e14e9663d304d8577427b368cb4710a20528690287a738b" dependencies = [ - "bitflags", + "bitflags 2.11.1", "bytes", "futures-util", "http", @@ -1838,7 +1951,7 @@ version = "0.244.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "47b807c72e1bac69382b3a6fb3dbe8ea4c0ed87ff5629b8685ae6b9a611028fe" dependencies = [ - "bitflags", + "bitflags 2.11.1", "hashbrown 0.15.5", "indexmap", "semver", @@ -2035,7 +2148,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9d66ea20e9553b30172b5e831994e35fbde2d165325bec84fc43dbf6f4eb9cb2" dependencies = [ "anyhow", - "bitflags", + "bitflags 2.11.1", "indexmap", "log", "serde", diff --git a/Cargo.toml b/Cargo.toml index 2b8f724..8373ee6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -20,6 +20,8 @@ futures = "0.3.31" clap = { version = "4", features = ["derive"] } thiserror = "2" dashmap = "6" +log = "0.4" +env_logger = "0.11" [features] default = [] diff --git a/src/commands/all.rs b/src/commands/all.rs index 5957cff..ca261ba 100644 --- a/src/commands/all.rs +++ b/src/commands/all.rs @@ -18,19 +18,14 @@ pub async fn run_all( max_wishlist: usize, ) -> Result<(), AppError> { let my_data = load_my_cars_or_warn(my_cars_path)?; - let (ctx, counts) = run_analysis(&my_data, cars_db_path, times_dir).await?; + let owned = my_data.owned.clone(); + let wishlist = my_data.wishlist.clone(); + let (ctx, counts) = run_analysis(my_data, cars_db_path, times_dir).await?; output::write_best_csv(output_dir, &counts)?; output::write_very_best_csv(output_dir, &counts, min_count)?; - output::write_trash_csv(output_dir, &counts, &my_data.owned)?; + output::write_trash_csv(output_dir, &counts, &owned)?; output::write_details_csv(output_dir, &ctx)?; - output::write_wishlist_files( - output_dir, - &counts, - &my_data.owned, - &my_data.wishlist, - min_count, - max_wishlist, - )?; + output::write_wishlist_files(output_dir, &counts, &owned, &wishlist, min_count, max_wishlist)?; output::write_improve_csv(output_dir, &counts)?; println!( "Written: best.csv, very_best.csv, trash.csv, details.csv, wishlist.csv, add.csv, remove.csv, improve.csv" diff --git a/src/commands/best.rs b/src/commands/best.rs index 1aa1d19..c96ccc1 100644 --- a/src/commands/best.rs +++ b/src/commands/best.rs @@ -17,7 +17,7 @@ pub async fn run_best( min_count: usize, ) -> Result<(), AppError> { let my_data = load_my_cars_or_warn(my_cars_path)?; - let (_ctx, counts) = run_analysis(&my_data, cars_db_path, times_dir).await?; + let (_ctx, counts) = run_analysis(my_data, cars_db_path, times_dir).await?; write_best_csv(output_dir, &counts)?; write_very_best_csv(output_dir, &counts, min_count)?; println!("Written: best.csv, very_best.csv"); diff --git a/src/commands/details.rs b/src/commands/details.rs index 90ccc66..561f6b9 100644 --- a/src/commands/details.rs +++ b/src/commands/details.rs @@ -16,7 +16,7 @@ pub async fn run_details( output_dir: &str, ) -> Result<(), AppError> { let my_data = load_my_cars_or_warn(my_cars_path)?; - let (ctx, _counts) = run_analysis(&my_data, cars_db_path, times_dir).await?; + let (ctx, _counts) = run_analysis(my_data, cars_db_path, times_dir).await?; write_details_csv(output_dir, &ctx)?; println!("Written: details.csv"); Ok(()) diff --git a/src/commands/improve.rs b/src/commands/improve.rs index c533f7e..1da9803 100644 --- a/src/commands/improve.rs +++ b/src/commands/improve.rs @@ -16,7 +16,7 @@ pub async fn run_improve( output_dir: &str, ) -> Result<(), AppError> { let my_data = load_my_cars_or_warn(my_cars_path)?; - let (_ctx, counts) = run_analysis(&my_data, cars_db_path, times_dir).await?; + let (_ctx, counts) = run_analysis(my_data, cars_db_path, times_dir).await?; let count = write_improve_csv(output_dir, &counts)?; if count == 0 { println!("No cars to improve — all owned cars are already in final or maxed configs."); diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 85f38a5..f67a0d5 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -33,8 +33,8 @@ pub fn load_my_cars_or_warn(path: &str) -> Result { match load_my_cars_data(path) { Ok(data) => Ok(data), Err(AppError::MissingFile(msg)) => { - eprintln!("Warning: {msg}"); - eprintln!(" Analysis will run without ownership data."); + log::warn!("{msg}"); + log::warn!("Analysis will run without ownership data."); Ok(MyCarsData::default()) } Err(e) => Err(e), @@ -45,23 +45,24 @@ pub fn load_my_cars_or_warn(path: &str) -> Result { // Analysis orchestration // --------------------------------------------------------------------------- -/// Common analysis setup: loads user data, parses all timing files, and computes car counts. +/// Common analysis setup: parses all timing files and computes car counts. +/// +/// Takes `my_data` by value: `owned` is shared with the parse tasks via `Arc`, +/// and `owned_configs`/`copy_count` are moved into [`CarCounts`]. Callers that +/// need `owned` or `wishlist` after analysis must retain their own copy before +/// calling this function. /// /// Returns the analysis context (for stdout commands) and the aggregated car counts -/// (for CSV output commands). The car list is stored inside `CarCounts`. +/// (for CSV output commands). pub async fn run_analysis( - my_data: &MyCarsData, + my_data: MyCarsData, cars_db_path: &str, times_dir: &str, ) -> Result<(AnalysisContext, CarCounts), AppError> { let ctx = AnalysisContext::new(); - let owned = Arc::new(my_data.owned.clone()); + let MyCarsData { owned, wishlist: _, owned_configs, copy_count } = my_data; + let owned = Arc::new(owned); let car_list = parse_all_times(owned, &ctx, cars_db_path, times_dir).await?; - let counts = compute_car_counts( - &ctx, - car_list, - my_data.owned_configs.clone(), - my_data.copy_count.clone(), - ); + let counts = compute_car_counts(&ctx, car_list, owned_configs, copy_count); Ok((ctx, counts)) } diff --git a/src/commands/track.rs b/src/commands/track.rs index 9b77189..e4fc372 100644 --- a/src/commands/track.rs +++ b/src/commands/track.rs @@ -9,19 +9,16 @@ use crate::output::print_best_car; /// Executes the track command: shows best car for a specific track/condition/RQ. /// -/// Note: `_output_dir` is unused because the track command prints to stdout -/// rather than writing a CSV file. It exists for API consistency with other -/// command signatures so the caller doesn't need per-command branching. +/// Prints to stdout rather than writing a CSV file, so no output directory is needed. pub async fn run_track( my_cars_path: &str, cars_db_path: &str, times_dir: &str, - _output_dir: &str, track: &str, condition: &str, rq: u8, ) -> Result<(), AppError> { let my_data = load_my_cars_or_warn(my_cars_path)?; - let (ctx, _counts) = run_analysis(&my_data, cars_db_path, times_dir).await?; + let (ctx, _counts) = run_analysis(my_data, cars_db_path, times_dir).await?; print_best_car(&ctx, track, condition, rq) } diff --git a/src/commands/trash.rs b/src/commands/trash.rs index 7fcfc6e..53eb0a7 100644 --- a/src/commands/trash.rs +++ b/src/commands/trash.rs @@ -17,8 +17,9 @@ pub async fn run_trash( output_dir: &str, ) -> Result<(), AppError> { let my_data = load_my_cars_or_warn(my_cars_path)?; - let (_ctx, counts) = run_analysis(&my_data, cars_db_path, times_dir).await?; - write_trash_csv(output_dir, &counts, &my_data.owned)?; + let owned = my_data.owned.clone(); + let (_ctx, counts) = run_analysis(my_data, cars_db_path, times_dir).await?; + write_trash_csv(output_dir, &counts, &owned)?; println!("Written: trash.csv"); Ok(()) } diff --git a/src/commands/wishlist.rs b/src/commands/wishlist.rs index e02801e..ce72dc8 100644 --- a/src/commands/wishlist.rs +++ b/src/commands/wishlist.rs @@ -17,15 +17,10 @@ pub async fn run_wishlist( max_wishlist: usize, ) -> Result<(), AppError> { let my_data = load_my_cars_or_warn(my_cars_path)?; - let (_ctx, counts) = run_analysis(&my_data, cars_db_path, times_dir).await?; - write_wishlist_files( - output_dir, - &counts, - &my_data.owned, - &my_data.wishlist, - min_count, - max_wishlist, - )?; + let owned = my_data.owned.clone(); + let wishlist = my_data.wishlist.clone(); + let (_ctx, counts) = run_analysis(my_data, cars_db_path, times_dir).await?; + write_wishlist_files(output_dir, &counts, &owned, &wishlist, min_count, max_wishlist)?; println!("Written: wishlist.csv, add.csv, remove.csv"); Ok(()) } diff --git a/src/context.rs b/src/context.rs index a8064a1..18431b3 100644 --- a/src/context.rs +++ b/src/context.rs @@ -50,9 +50,9 @@ pub struct CarCounts { pub guid_to_rid: HashMap>, /// Map from guid → Car (for O(1) lookups of car metadata by GUID). /// Built once in `compute_car_counts()` and reused by trash/wishlist output. + /// Sole owner of the car database — iterate `car_map.values()` where a + /// car list was previously used. pub car_map: HashMap, - /// Full car list from the database (kept for any callers that need iteration). - pub car_list: Vec, /// Actual tune configs for each owned car (guid → list of configs from all copies). /// Used by the improve command to determine which cars are not yet at a final build. pub owned_configs: HashMap>, @@ -94,10 +94,10 @@ pub fn compute_car_counts( } // Build guid → Car lookup once, reused by trash/wishlist output functions. - let car_map: HashMap = - car_list.iter().map(|c| (c.guid.clone(), c.clone())).collect(); + // Consumes car_list so each Car is stored exactly once. + let car_map: HashMap = car_list.into_iter().map(|c| (c.guid.clone(), c)).collect(); - CarCounts { by_rid, guid_to_rid, car_map, car_list, owned_configs, copy_count } + CarCounts { by_rid, guid_to_rid, car_map, owned_configs, copy_count } } #[cfg(test)] @@ -158,7 +158,7 @@ mod tests { // car_map should have the car assert!(counts.car_map.contains_key("guid_test")); - assert_eq!(counts.car_list.len(), 1); + assert_eq!(counts.car_map.len(), 1); } #[test] diff --git a/src/http.rs b/src/http.rs index f69253f..c76a593 100644 --- a/src/http.rs +++ b/src/http.rs @@ -9,6 +9,11 @@ use reqwest::get; use crate::types::AppError; +/// Linear backoff between retry attempts: 500ms, 1000ms, 1500ms, ... +fn retry_delay(attempt: usize) -> std::time::Duration { + std::time::Duration::from_millis(500 * (attempt as u64 + 1)) +} + /// Downloads a file from a URL and saves it locally. /// /// Returns `Ok(())` on success, or an error describing what went wrong. @@ -39,15 +44,12 @@ pub async fn get_file(file_url: &str, local_file_path: &str) -> Result<(), AppEr if !status.is_success() { // Only retry on server errors (5xx); client errors (4xx) are permanent. if status.is_server_error() && attempt < MAX_FETCH_RETRIES { - eprintln!( - "Warning: HTTP {} from {file_url} (attempt {}/{MAX_FETCH_RETRIES}), retrying...", + log::warn!( + "HTTP {} from {file_url} (attempt {}/{MAX_FETCH_RETRIES}), retrying...", status, attempt + 1, ); - tokio::time::sleep(std::time::Duration::from_millis( - 500 * (attempt + 1) as u64, - )) - .await; + tokio::time::sleep(retry_delay(attempt)).await; attempt += 1; continue; } @@ -57,14 +59,11 @@ pub async fn get_file(file_url: &str, local_file_path: &str) -> Result<(), AppEr Ok(content) => { if content.is_empty() { if attempt < MAX_FETCH_RETRIES { - eprintln!( - "Warning: empty response from {file_url} (attempt {}/{MAX_FETCH_RETRIES}), retrying...", + log::warn!( + "empty response from {file_url} (attempt {}/{MAX_FETCH_RETRIES}), retrying...", attempt + 1, ); - tokio::time::sleep(std::time::Duration::from_millis( - 500 * (attempt + 1) as u64, - )) - .await; + tokio::time::sleep(retry_delay(attempt)).await; attempt += 1; continue; } @@ -84,14 +83,11 @@ pub async fn get_file(file_url: &str, local_file_path: &str) -> Result<(), AppEr } Err(error) => { if attempt < MAX_FETCH_RETRIES { - eprintln!( - "Warning: failed to fetch {file_url} (attempt {}/{MAX_FETCH_RETRIES}): {error}", + log::warn!( + "failed to fetch {file_url} (attempt {}/{MAX_FETCH_RETRIES}): {error}", attempt + 1, ); - tokio::time::sleep(std::time::Duration::from_millis( - 500 * (attempt + 1) as u64, - )) - .await; + tokio::time::sleep(retry_delay(attempt)).await; attempt += 1; continue; } diff --git a/src/output/mod.rs b/src/output/mod.rs index d03c81b..9d94812 100644 --- a/src/output/mod.rs +++ b/src/output/mod.rs @@ -25,16 +25,25 @@ const TRASH_MIN_RQ: u8 = 80; // Sorting helpers // --------------------------------------------------------------------------- +/// Field-wise ordering shared by record-based CSV outputs: +/// RQ, then normalized config, then brand, then model name. +fn compare_by_rq_config_brand_name( + (rq_a, config_a, brand_a, name_a): (&u8, &str, &str, &str), + (rq_b, config_b, brand_b, name_b): (&u8, &str, &str, &str), +) -> std::cmp::Ordering { + rq_a.cmp(rq_b).then(config_a.cmp(config_b)).then(brand_a.cmp(brand_b)).then(name_a.cmp(name_b)) +} + /// Comparison function for sorting `BestCarRecord`s by RQ, then normalized config, /// then brand, then model name. /// /// Avoids cloning into a `Car` just for sorting — uses the same ordering as /// `compare_cars` but operates directly on `BestCarRecord`. pub fn compare_records(a: &BestCarRecord, b: &BestCarRecord) -> std::cmp::Ordering { - a.rq.cmp(&b.rq) - .then(a.normalized_config.cmp(&b.normalized_config)) - .then(a.brand.cmp(&b.brand)) - .then(a.only_name.cmp(&b.only_name)) + compare_by_rq_config_brand_name( + (&a.rq, &a.normalized_config, &a.brand, &a.only_name), + (&b.rq, &b.normalized_config, &b.brand, &b.only_name), + ) } // --------------------------------------------------------------------------- @@ -167,9 +176,14 @@ pub fn write_very_best_csv( /// 2. Its actual tune config is a final/maxed build (sum ≥ 8) that is not /// best on any track — even if other configs of the same car model are best. /// -/// Non-final builds (sum < 8) are excluded from category 2 because they +/// Non-final builds are excluded from category 2 because they /// belong in `improve.csv` instead. Prize cars and RQ ≥ 80 are always excluded. /// +/// **Known gap:** an owned non-final car that is not best on any track in *any* +/// config appears in neither trash.csv (not final yet) nor improve.csv (no best +/// config to suggest as a target). Such cars stay under the radar until they +/// either reach a final build or are manually reviewed. +/// /// Format: RQ,Brand,Model,Year,Config pub fn write_trash_csv( output_dir: &str, @@ -286,7 +300,9 @@ pub fn write_details_csv(output_dir: &str, ctx: &AnalysisContext) -> Result<(), /// /// A car qualifies only when **one config** reaches `min_count` tracks — counts are /// NOT summed across different configs. If multiple configs for the same car qualify, -/// the one with the highest count is kept as the representative entry. +/// the one with the highest count is kept as the representative entry; on equal +/// counts the first record seen in `by_rid` iteration order wins (deterministic +/// but arbitrary, since `by_rid` is a `HashMap`). /// /// **Owned cars** are included only if the player has a **final build** that differs /// from the qualifying config. This allows wishlisting a second copy for a different @@ -454,9 +470,7 @@ pub fn write_wishlist_files( }) .collect(); if dropped_count > 0 { - eprintln!( - "Warning: {dropped_count} wishlist car(s) not found in cars database and omitted from remove.csv" - ); + log::warn!("{dropped_count} wishlist car(s) not found in cars database and omitted from remove.csv"); } write_sorted_csv( @@ -465,10 +479,10 @@ pub fn write_wishlist_files( "RQ,Brand,Model,Year,FinalBuild,Count", &to_remove, |a, b| { - a.rq.cmp(&b.rq) - .then(a.normalized_config.cmp(&b.normalized_config)) - .then(a.brand.cmp(&b.brand)) - .then(a.only_name.cmp(&b.only_name)) + compare_by_rq_config_brand_name( + (&a.rq, &a.normalized_config, &a.brand, &a.only_name), + (&b.rq, &b.normalized_config, &b.brand, &b.only_name), + ) }, |entry, file| { writeln!( @@ -519,14 +533,19 @@ pub fn write_improve_csv(output_dir: &str, counts: &CarCounts) -> Result = counts + // Borrowed keys/values — the hot-path membership check below allocates nothing. + let owned_final_configs: HashMap<&str, HashSet<&str>> = counts .owned_configs .iter() .filter_map(|(guid, configs)| counts.car_map.get(guid).map(|car| (car, configs))) .flat_map(|(car, configs)| { - configs.iter().filter(|c| is_final_build(c)).map(|c| (car.rid.clone(), c.clone())) + let rid = car.rid.as_str(); + configs.iter().filter(|c| is_final_build(c)).map(move |c| (rid, c.as_str())) }) - .collect(); + .fold(HashMap::new(), |mut map, (rid, config)| { + map.entry(rid).or_default().insert(config); + map + }); // Iterate over owned cars from the player's actual config data. // For each owned car, check if its actual config is NOT a final build. @@ -540,25 +559,25 @@ pub fn write_improve_csv(output_dir: &str, counts: &CarCounts) -> Result content, + // File not found is expected for most cars (no timing data recorded) — silent skip. + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(()), Err(e) => { - // File not found is expected for some cars — treat as non-fatal - eprintln!("Warning: could not read {local_file_path}: {e}"); + log::warn!("could not read {local_file_path}: {e}"); return Ok(()); } }; @@ -106,7 +107,7 @@ pub fn parse_car_times( // Process each configuration variant (111, 233, 323, etc.) if let Some(configurations) = json_data["data"].as_object() { - for (raw_config, entry) in configurations { + for (raw_config, config_data) in configurations { // Strip version prefix (e.g., "v2c_" from "v2c_233", "v2b_" from "v2b_111") // at read time. Downstream code only ever sees bare digit strings. // Generic approach: find the last '_' and take what follows it, @@ -121,13 +122,20 @@ pub fn parse_car_times( .map(|(_, suffix)| suffix.to_string()) .unwrap_or_else(|| raw_config.clone()); // Process timing data for each track - if let Some(times) = entry["times"].as_object() { + if let Some(times) = config_data["times"].as_object() { for (track_id, details) in times { // Parse track ID to separate base track name from surface condition. // Use rsplit_once to split on the LAST "_a" so track names that // contain "_a" (e.g. "airplaneHangars") are handled correctly. if let Some((cleaned_track_id, track_type)) = track_id.rsplit_once("_a") { - let mut time = details["t"].as_f64().unwrap_or(0.0); + let Some(mut time) = details["t"].as_f64() else { + log::warn!( + "skipping {}/{}: missing or non-numeric time field", + car.rid, + track_id + ); + continue; + }; let down_votes = details["down"].as_array(); let no_down_votes = match down_votes { @@ -302,12 +310,12 @@ pub async fn parse_all_times( match result { // Outer JoinError from tokio::spawn Err(e) => { - eprintln!("Warning: parsing task failed: {e}"); + log::warn!("parsing task failed: {e}"); parse_errors += 1; } // parse_car_times or semaphore returned an error Ok(Err(e)) => { - eprintln!("Warning: {e}"); + log::warn!("{e}"); parse_errors += 1; } // parse_car_times succeeded @@ -315,7 +323,7 @@ pub async fn parse_all_times( } } if parse_errors > 0 { - eprintln!("Warning: {parse_errors} car(s) had parse errors during analysis"); + log::warn!("{parse_errors} car(s) had parse errors during analysis"); } // If every car failed to parse, the data is unusable — propagate the error. @@ -364,14 +372,14 @@ pub async fn fetch_times(cars_db_path: &str, times_dir: &str) -> Result<(), AppE match result { Ok(Ok(())) => {} Ok(Err(e)) => { - eprintln!("{e}"); + log::warn!("{e}"); if first_error.is_none() { first_error = Some(e); } } Err(e) => { let msg = AppError::Network(format!("Task join error: {e}")); - eprintln!("{msg}"); + log::warn!("{msg}"); if first_error.is_none() { first_error = Some(msg); } diff --git a/src/records.rs b/src/records.rs index 626fa33..ee3f435 100644 --- a/src/records.rs +++ b/src/records.rs @@ -150,6 +150,9 @@ enum Commands { /// ``` #[tokio::main] async fn main() { + // Default to warn level so library/bin warnings stay visible without RUST_LOG; + // RUST_LOG can still silence or raise verbosity. + env_logger::Builder::from_env(env_logger::Env::default().default_filter_or("warn")).init(); if let Err(e) = run().await { eprintln!("Error: {e}"); std::process::exit(1); @@ -217,7 +220,6 @@ async fn run() -> Result<(), AppError> { &cli.my_cars, &cli.cars_db, &cli.times_dir, - &cli.output_dir, &track, &condition, rq, diff --git a/src/types.rs b/src/types.rs index b1a57dc..2d18f57 100644 --- a/src/types.rs +++ b/src/types.rs @@ -132,7 +132,7 @@ pub fn normalize_to_final_build(config: &str) -> String { digits[i] = b - b'0'; } - let sum: i32 = digits.iter().map(|&d| d as i32).sum(); + let sum: u8 = digits.iter().sum(); if sum >= 8 { // Already a final build (sum == 8) or maxed (sum > 8) — return as-is. return config.to_string(); @@ -152,8 +152,15 @@ pub fn normalize_to_final_build(config: &str) -> String { /// Returns `true` if the config is a final build (digit sum == 8) or maxed (sum > 8). /// /// Configs with sum < 8 are non-final and still have upgrade steps remaining. -/// Uses [`normalize_to_final_build`] internally: if normalization returns the -/// original string unchanged, the config is already at or beyond a final build. +/// Malformed configs (not exactly 3 ASCII digits) are treated as non-final so +/// they surface in improve.csv instead of being silently classified as finished. +/// +/// Checks the digits directly — no allocation, unlike going through +/// [`normalize_to_final_build`]. pub fn is_final_build(config: &str) -> bool { - normalize_to_final_build(config) == config + let bytes = config.as_bytes(); + if bytes.len() != 3 || !bytes.iter().all(|&b| b.is_ascii_digit()) { + return false; + } + bytes.iter().map(|&b| b - b'0').sum::() >= 8 } diff --git a/src/user_data.rs b/src/user_data.rs index f3037db..39d119a 100644 --- a/src/user_data.rs +++ b/src/user_data.rs @@ -79,7 +79,7 @@ pub fn load_my_cars_data(path: &str) -> Result { let config = format!("{e}{w}{c}"); owned_configs.entry(guid.to_string()).or_default().push(config); } else { - eprintln!("Warning: locked card {guid} has incomplete tune data — skipped"); + log::warn!("locked card {guid} has incomplete tune data — skipped"); } } }