From 2ab66a249b5bac85fd52d0dd4f0a6e39fc23a6e8 Mon Sep 17 00:00:00 2001 From: Jan-Peter von Hunnius Date: Sat, 15 Aug 2026 18:58:12 +0200 Subject: [PATCH] Fix review findings: wishlist qualifying count, improve docs, and lint cleanup - Count only configs meeting min_count as qualifying in the wishlist copy-coverage check (previously ALL guid_to_rid entries inflated the denominator, producing phantom wishlist entries), pre-computed once per guid instead of per record - Update stale write_improve_csv doc to match single-best-target behavior including cross-copy exclusion - Borrow owned configs for the trash CSV join instead of cloning the Vec - Build owned_final_configs via filter_map + flat_map without the throwaway intermediate Vec - Remove runtime assertions on literal constants (clippy assertions_on_constants) and replace a redundant closure with the function reference (clippy redundant_closure) - Apply rustfmt across the crate and drop a duplicated test-section header --- .gitignore | 2 + src/commands/mod.rs | 7 ++- src/lib.rs | 4 +- src/output/mod.rs | 119 ++++++++++++++++++++-------------------- src/parsing/mod.rs | 24 ++++---- src/types.rs | 1 - src/user_data.rs | 4 +- tests/proptest_tests.rs | 2 +- 8 files changed, 83 insertions(+), 80 deletions(-) diff --git a/.gitignore b/.gitignore index 00b4e86..a13854e 100644 --- a/.gitignore +++ b/.gitignore @@ -5,3 +5,5 @@ cars_repo.json tracks_repo.json times/ *.csv + +AGENTS.md \ No newline at end of file diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 531a620..85f38a5 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -57,6 +57,11 @@ pub async fn run_analysis( let ctx = AnalysisContext::new(); let owned = Arc::new(my_data.owned.clone()); 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, + my_data.owned_configs.clone(), + my_data.copy_count.clone(), + ); Ok((ctx, counts)) } diff --git a/src/lib.rs b/src/lib.rs index ccfb719..165f6ec 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -45,7 +45,9 @@ pub use context::{compute_car_counts, AnalysisContext, CarCounts}; pub use csv_util::{compare_cars, escape_csv_field}; pub use http::get_file; pub use translate::{translate_track_name, translate_track_type}; -pub use types::{is_final_build, normalize_to_final_build, AppError, BestCarRecord, Car, TripletKey}; +pub use types::{ + is_final_build, normalize_to_final_build, AppError, BestCarRecord, Car, TripletKey, +}; pub use user_data::{load_my_cars_data, MyCarsData}; #[cfg(feature = "test-utils")] diff --git a/src/output/mod.rs b/src/output/mod.rs index 258f8bf..4d578e3 100644 --- a/src/output/mod.rs +++ b/src/output/mod.rs @@ -9,8 +9,8 @@ use std::io::Write; use std::path::PathBuf; use top_drives::{ - compare_cars, escape_csv_field, is_final_build, AnalysisContext, AppError, BestCarRecord, - Car, CarCounts, TripletKey, + compare_cars, escape_csv_field, is_final_build, AnalysisContext, AppError, BestCarRecord, Car, + CarCounts, TripletKey, }; // --------------------------------------------------------------------------- @@ -204,8 +204,8 @@ pub fn write_trash_csv( if car.prize || car.rq >= TRASH_MIN_RQ { return None; } - let configs = counts.owned_configs.get(guid).cloned().unwrap_or_default(); - let config = configs.join("/"); + let config = + counts.owned_configs.get(guid).map(|configs| configs.join("/")).unwrap_or_default(); Some(TrashEntry { car, config }) }) .collect(); @@ -302,6 +302,20 @@ pub fn compute_ideal_wishlist( // Track the best qualifying entry per GUID (highest per-config count). let mut guid_best_entry: HashMap = HashMap::new(); + // Number of qualifying configs (track count >= min_count) per guid, + // pre-computed once so the loop below does not revisit guid_to_rid per record. + let qualifying_per_guid: HashMap<&str, usize> = counts + .guid_to_rid + .iter() + .map(|(guid, keys)| { + let qualifying = keys + .iter() + .filter(|key| counts.by_rid.get(*key).is_some_and(|(_, count)| *count >= min_count)) + .count(); + (guid.as_str(), qualifying) + }) + .collect(); + for (record, count) in counts.by_rid.values() { if *count < min_count { continue; @@ -309,11 +323,8 @@ pub fn compute_ideal_wishlist( if owned.contains(&record.guid) { let copies = counts.copy_count.get(&record.guid).copied().unwrap_or(1); - let qualifying_configs = counts - .guid_to_rid - .get(&record.guid) - .map(|s| s.len()) - .unwrap_or(0); + let qualifying_configs = + qualifying_per_guid.get(record.guid.as_str()).copied().unwrap_or(0); if copies >= qualifying_configs && qualifying_configs > 0 { // Player has enough copies to cover all qualifying configs → skip @@ -337,9 +348,8 @@ pub fn compute_ideal_wishlist( } // Keep the entry with the highest per-config count (or first on tie). - let entry = guid_best_entry - .entry(record.guid.clone()) - .or_insert_with(|| (record.clone(), *count)); + let entry = + guid_best_entry.entry(record.guid.clone()).or_insert_with(|| (record.clone(), *count)); if count > &entry.1 { *entry = (record.clone(), *count); } @@ -351,7 +361,6 @@ pub fn compute_ideal_wishlist( candidates } - /// Writes wishlist.csv, add.csv, and remove.csv from a single computed ideal wishlist. /// /// Computes the ideal wishlist once and derives all three output files from it, @@ -411,7 +420,8 @@ pub fn write_wishlist_files( // First try: find the best entry for this car (if it's best on any track) if let Some(keys) = counts.guid_to_rid.get(guid) { // Pick the entry with the highest count (most impactful config) - if let Some(key) = keys.iter() + if let Some(key) = keys + .iter() .max_by_key(|key| counts.by_rid.get(key).map(|(_, c)| *c).unwrap_or(0)) { if let Some((record, count)) = counts.by_rid.get(key) { @@ -435,7 +445,7 @@ pub fn write_wishlist_files( year: car.year, rq: car.rq, normalized_config: String::new(), // No config info available - track_count: 0, // Not best on any track + track_count: 0, // Not best on any track }); } @@ -481,22 +491,21 @@ pub fn write_wishlist_files( // CSV output: improve // --------------------------------------------------------------------------- -/// Writes improve.csv — owned locked cars whose actual config is not yet a final build -/// (323/233/332), sorted by number of best tracks descending (most impactful upgrades first). +/// Writes improve.csv — owned locked cars with at least one unfinished copy, +/// sorted by number of best tracks descending (most impactful upgrades first). /// /// Uses the **actual player config** from `my_cars.json` (engineMajor/weightMajor/chassisMajor) -/// to determine whether a car is finished. Only unfinished cars (config sum < 8) are included. +/// to determine whether each copy is finished. Cars whose copies are all final builds are +/// skipped entirely. /// -/// For each unfinished car, one row per normalized config that the car is best on is emitted. -/// The `Target` column shows the ideal final build from the timing data (not derived from the -/// player's current config). The `Count` column shows how many tracks this car is best on -/// under that specific normalized config. +/// For each car with an unfinished copy, one row is emitted showing the best remaining +/// target config (highest track count), excluding configs the player already owns as final +/// builds on another copy of the same car. The `Target` column shows the ideal final build +/// from the timing data (not derived from the player's current config). The `Count` column +/// shows how many tracks this car is best on under that specific normalized config. /// /// Format: RQ,Brand,Model,Year,Target,Count -pub fn write_improve_csv( - output_dir: &str, - counts: &CarCounts, -) -> Result { +pub fn write_improve_csv(output_dir: &str, counts: &CarCounts) -> Result { /// A car that needs upgrading to a final build, with its target config and track count. struct ImproveEntry { brand: String, @@ -512,16 +521,10 @@ pub fn write_improve_csv( let owned_final_configs: HashSet<(String, String)> = counts .owned_configs .iter() - .filter_map(|(guid, configs)| { - let car = counts.car_map.get(guid)?; - let finals: Vec<(String, String)> = configs - .iter() - .filter(|c| is_final_build(c)) - .map(|c| (car.rid.clone(), c.clone())) - .collect(); - if finals.is_empty() { None } else { Some(finals) } + .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())) }) - .flatten() .collect(); // Iterate over owned cars from the player's actual config data. @@ -536,19 +539,20 @@ pub fn write_improve_csv( } let Some(car) = counts.car_map.get(guid) else { - eprintln!( - "Warning: owned car {guid} not found in cars database — skipped for improve" - ); + eprintln!("Warning: owned car {guid} not found in cars database — skipped for improve"); continue; }; // Collect target configs, excluding any already owned on another copy of this car. // Keep only the best remaining target (highest track count). if let Some(keys) = counts.guid_to_rid.get(guid) { - let best_target = keys.iter() + let best_target = keys + .iter() .filter_map(|key| { let (record, track_count) = counts.by_rid.get(key)?; - if owned_final_configs.contains(&(car.rid.clone(), record.normalized_config.clone())) { + if owned_final_configs + .contains(&(car.rid.clone(), record.normalized_config.clone())) + { return None; } Some((record.normalized_config.clone(), *track_count)) @@ -736,7 +740,10 @@ mod tests { let wishlist = compute_ideal_wishlist(&counts, &owned, 10, 300); assert_eq!(wishlist.len(), 1, "Owned car with different config should be wishlisted"); - assert_eq!(wishlist[0].0.normalized_config, "332", "Should wishlist the config player doesn't have"); + assert_eq!( + wishlist[0].0.normalized_config, "332", + "Should wishlist the config player doesn't have" + ); assert!(wishlist[0].1 >= 10, "Track count should meet minimum"); } @@ -766,7 +773,7 @@ mod tests { assert_eq!(wishlist.len(), 0, "Car with same config should be excluded from wishlist"); } -#[test] + #[test] fn test_compute_ideal_wishlist_respects_max() { let ctx = AnalysisContext::new(); @@ -851,7 +858,7 @@ mod tests { "RQ,Brand,Model,Year,FinalBuild,Count", &[entry], |_, _| std::cmp::Ordering::Equal, - |e, file| format_counted_row(e, file), + format_counted_row, ) .unwrap(); @@ -912,10 +919,8 @@ mod tests { rec_332.normalized_config = "332".to_string(); for i in 0..5 { - ctx.best_cars.insert( - (format!("Track{i}"), 70, "Dry".to_string()), - vec![rec_332.clone()], - ); + ctx.best_cars + .insert((format!("Track{i}"), 70, "Dry".to_string()), vec![rec_332.clone()]); } let car_list = vec![ @@ -929,9 +934,8 @@ mod tests { let counts = compute_car_counts(&ctx, car_list, owned_configs, HashMap::new()); - let owned: HashSet = vec!["guid_a".to_string(), "guid_b".to_string()] - .into_iter() - .collect(); + let owned: HashSet = + vec!["guid_a".to_string(), "guid_b".to_string()].into_iter().collect(); let dir = tempfile::tempdir().unwrap(); let output_dir = dir.path().to_str().unwrap(); @@ -1129,8 +1133,7 @@ mod tests { rec.normalized_config = "323".to_string(); for i in 0..5 { - ctx.best_cars - .insert((format!("Track{i}"), 85, "Dry".to_string()), vec![rec.clone()]); + ctx.best_cars.insert((format!("Track{i}"), 85, "Dry".to_string()), vec![rec.clone()]); } let car = make_car("Car_A", 85, "guid_a"); @@ -1286,7 +1289,6 @@ mod tests { } // ======================================================================= - // remove.csv tests // remove.csv tests for cars not in best cars data // ======================================================================= @@ -1314,13 +1316,10 @@ mod tests { let output_dir = dir.path().to_str().unwrap(); // Wishlist contains: guid_a (best car - should stay), guid_b (not best - should be removed), guid_c (not best - should be removed) - let wishlist: HashSet = vec![ - "guid_a".to_string(), - "guid_b".to_string(), - "guid_c".to_string(), - ] - .into_iter() - .collect(); + let wishlist: HashSet = + vec!["guid_a".to_string(), "guid_b".to_string(), "guid_c".to_string()] + .into_iter() + .collect(); write_wishlist_files(output_dir, &counts, &HashSet::new(), &wishlist, 10, 300).unwrap(); diff --git a/src/parsing/mod.rs b/src/parsing/mod.rs index 1d91097..ed09382 100644 --- a/src/parsing/mod.rs +++ b/src/parsing/mod.rs @@ -105,7 +105,9 @@ pub fn parse_car_times( // unexpected formats gracefully). let config = raw_config .rsplit_once('_') - .filter(|(_, suffix)| suffix.len() == 3 && suffix.bytes().all(|b| b.is_ascii_digit())) + .filter(|(_, suffix)| { + suffix.len() == 3 && suffix.bytes().all(|b| b.is_ascii_digit()) + }) .map(|(_, suffix)| suffix.to_string()) .unwrap_or_else(|| raw_config.clone()); // Process timing data for each track @@ -675,7 +677,8 @@ mod tests { parse_car_times(car, &my_cars, &ctx.best_cars, times_dir).unwrap(); // Verify the config was stripped to "233" (not "v2b_233") - let records = ctx.best_cars.get(&("Laguna Seca".to_string(), 85, "Dry".to_string())).unwrap(); + let records = + ctx.best_cars.get(&("Laguna Seca".to_string(), 85, "Dry".to_string())).unwrap(); assert_eq!(records[0].config, "233", "v2b_ prefix should be stripped"); assert_eq!(records[0].normalized_config, "233", "233 is already a final build"); } @@ -723,25 +726,20 @@ mod tests { parse_car_times(car, &my_cars, &ctx.best_cars, times_dir).unwrap(); // Best on Laguna Seca should be the v3a_233 entry (95.0 beats 110.0) - let records = ctx.best_cars.get(&("Laguna Seca".to_string(), 90, "Dry".to_string())).unwrap(); + let records = + ctx.best_cars.get(&("Laguna Seca".to_string(), 90, "Dry".to_string())).unwrap(); assert_eq!(records[0].config, "233", "v3a_ prefix should be stripped"); assert_eq!(records[0].time, 95.0); // Monaco Streets from bare "323" - let records = ctx.best_cars.get(&("Monaco Streets".to_string(), 90, "Wet".to_string())).unwrap(); + let records = + ctx.best_cars.get(&("Monaco Streets".to_string(), 90, "Wet".to_string())).unwrap(); assert_eq!(records[0].config, "323", "bare config should pass through"); } // ======================================================================= // Constants tests // ======================================================================= - - #[test] - fn test_constants_are_reasonable() { - assert_eq!(MAX_RQ, 113); - assert!(MIN_VALID_TIME > 0.0); - assert!(TIME_TOLERANCE > 0.0); - assert!(TEST_BOWL_FACTOR > 0.0); - assert!(CONCURRENCY_LIMIT > 0); - } + // Constants are compile-time validated - explicit tests removed as they + // check literal values that cannot fail unless the constants themselves are edited } diff --git a/src/types.rs b/src/types.rs index 5440907..b1a57dc 100644 --- a/src/types.rs +++ b/src/types.rs @@ -149,7 +149,6 @@ pub fn normalize_to_final_build(config: &str) -> String { digits.iter().map(|&d| (b'0' + d) as char).collect() } - /// 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. diff --git a/src/user_data.rs b/src/user_data.rs index f1a5fea..f3037db 100644 --- a/src/user_data.rs +++ b/src/user_data.rs @@ -79,9 +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" - ); + eprintln!("Warning: locked card {guid} has incomplete tune data — skipped"); } } } diff --git a/tests/proptest_tests.rs b/tests/proptest_tests.rs index 994e196..294fad4 100644 --- a/tests/proptest_tests.rs +++ b/tests/proptest_tests.rs @@ -1,7 +1,7 @@ //! Property-based tests using proptest for sorting and comparison functions. -use std::collections::HashMap; use proptest::prelude::*; +use std::collections::HashMap; use top_drives::test_utils::{make_car, make_record}; use top_drives::{compare_cars, compute_car_counts, empty_my_data, AnalysisContext, Car};