Skip to content

Commit c6d8a8c

Browse files
authored
Complete CLI-to-crate unification: remove export duplication, verify CSV/GPX/event output (#25)
* refactor: complete CLI-to-crate unification by consolidating export layer - Move export functionality from CLI (src/main.rs) to library (src/export.rs) - Remove 541 lines of duplicated export code from main.rs - Reduce main.rs from ~1822 to ~1417 lines (22% reduction) - CLI now uses library export functions: export_to_csv, export_to_gpx, export_to_event - Enhance export_to_gpx with log_start_datetime parameter for accurate timestamps - Remove unused structs: CsvExportOptions, CsvFieldMap from main.rs - Update examples to use new export_to_gpx signature - Update AGENTS.md to document full unification completion Resolves issue #24: Complete CLI-to-Crate Unification All tests pass (38 total): - cargo clippy --all-targets --all-features -- -D warnings ✓ - cargo fmt --all -- --check ✓ - cargo test --verbose ✓ (38 passed) - cargo test --features=cli --verbose ✓ - cargo build --release ✓ (no warnings) * refactor: consolidate export status formatting and enhance tests - Extract format_export_path() helper to eliminate duplication across CSV, GPX, and Event export status messages (3 blocks -> 1 reusable function) - Document GPX timestamp performance considerations for large GPS traces - Enhance test_export_options() with comprehensive assertions for all fields: - Validate event, force_export, and output_dir in both explicit and default configs - Now covers all 5 ExportOptions fields (was 3 fields previously) - CSV output: 87/87 files remain byte-for-byte identical to master (verified) - Maintains CLI as thin wrapper over shared library export functions - No functional changes to export logic or output data * fix: use log.log_number directly in format_export_path for consistency - Change format_export_path parameter from log_index to log_number (1-based) - Use log.log_number directly instead of computed log_index + 1 - Aligns with export.rs behavior which uses log.log_number for file suffix - Eliminates fragile dual calculation that could drift if assumptions change - All call sites updated to pass log.log_number - No change to output data (87/87 CSV files still match master) * refactor: use PathBuf for cross-platform path assembly in status messages - Change format_export_path to return (PathBuf, PathBuf, PathBuf, PathBuf) for all export types - Compute full paths using Path::join() instead of manual string concatenation with '/' - Use PathBuf::display() for OS-aware path printing (e.g., backslashes on Windows) - Ensures printed paths match actual OS path separators, fixing mixed separator issue - Fallback values ("blackbox") already consistent with export.rs - All call sites updated to use dedicated path variables for clarity - CSV output: 87/87 files remain byte-for-byte identical to master * refactor: centralize export path computation to shared library helper IMPLEMENTATION (De-duplication Goal) - Create compute_export_paths() helper in export.rs as single source of truth - Helper returns (csv, headers, gpx, event) paths with consistent naming rules - CLI now imports and uses compute_export_paths() instead of duplicating logic - Removes local format_export_path() function from main.rs (was duplicating logic) BENEFITS - Single source of truth: filename/suffix logic lives only in export.rs - Eliminates drift risk: if naming conventions change, update one place - Better maintainability: path construction logic no longer duplicated - Enables future enhancement: can extend naming rules without sync issues TECHNICAL DETAILS - compute_export_paths: centralizes base_name extraction, output_dir selection, log_suffix calculation, and path assembly using Path::join() for OS compatibility - Consistent fallback: uses "blackbox" as filename fallback (matches export.rs) - Used by all three export paths: CSV, GPX, Event exports - CLI status messages now derive paths from shared helper VERIFICATION ✅ cargo fmt --all -- --check : PASS ✅ cargo clippy --all-targets : PASS (no warnings) ✅ cargo test --verbose : PASS (37 tests) ✅ cargo build --release : PASS ✅ CSV output vs master : PASS (87/87 files identical) ✅ Multi-log file handling : PASS (correct .NN suffixes) IMPACT ON GOALS - De-duplication: ✅ Removed duplicate path construction logic from CLI - Shared library: ✅ compute_export_paths is now library API - Unified behavior: ✅ Both export functions and CLI use same path logic * refactor: remove unnecessary path computation from export_to_gpx early return IMPROVES SEPARATION OF CONCERNS - export_to_gpx() should only export data, not compute paths for messaging - Removed unnecessary path computation when gps_coordinates is empty - CLI already uses compute_export_paths() for message generation - Export functions stay focused on their core responsibility BENEFITS - Cleaner library API: export functions don't couple to CLI messaging needs - Better separation of concerns: path computation is CLI responsibility - More reusable: export functions can be used by other tools without side effects - Supports goal: Moving to shared libraries for both CLI and crate usage IMPLEMENTATION - Removed ~20 lines of path computation code from export_to_gpx early return - CLI message generation unchanged (already uses compute_export_paths) - export_to_event was already correct (had early return without path computation) VERIFICATION ✅ cargo fmt --all -- --check : PASS ✅ cargo clippy --all-targets : PASS ✅ cargo test --verbose : PASS (37 tests) ✅ cargo build --release : PASS ✅ CSV output vs master : PASS (87/87 files identical) ✅ GPX/event messaging : PASS (messages still displayed correctly) ALIGNMENT WITH GOALS ✅ De-duplication: Further reduced code duplication ✅ Shared libraries: Export functions now cleaner library APIs ✅ Better separation: Export functions don't depend on CLI messaging * refactor: unify base filename fallback to "blackbox" across all exports ADDRESSES INCONSISTENCY - Issue: export_to_gpx and export_to_event used "unknown" fallback compute_export_paths and export_to_csv used "blackbox" This violated the contract: "ensures CLI status messages match actual filenames" IMPLEMENTATION - Created extract_base_name() helper for consistent filename derivation - All export functions now use this shared helper - Guarantees consistent "blackbox" fallback everywhere - Eliminates duplication of base name extraction logic BENEFITS FOR DEDUPLICATION GOAL ✅ Single source of truth: extract_base_name() is used by: - compute_export_paths() for CLI message prediction - export_to_csv() for actual CSV output - export_to_gpx() for actual GPX output - export_to_event() for actual event output ✅ Prevents drift: If base name logic changes, one place to update ✅ Reduces duplication: Shared helper eliminates 4 copies of same logic ✅ Supports library API: Export functions now truly match predictions VERIFICATION ✅ cargo fmt --all -- --check : PASS ✅ cargo clippy --all-targets : PASS (no warnings) ✅ cargo test --verbose : PASS (37 tests) ✅ cargo build --release : PASS ✅ CSV output vs master : PASS (87/87 files identical) IMPACT ON GOALS ✅ De-duplication: Removed duplicate base name extraction (4 copies -> 1) ✅ Shared libraries: All exports use same fallback, guaranteeing consistency ✅ Single source of truth: One place to define filename conventions
1 parent 1711b6f commit c6d8a8c

6 files changed

Lines changed: 175 additions & 544 deletions

File tree

AGENTS.md

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,15 +13,14 @@
1313

1414
## Architecture & Code Organization
1515
- **Library-first design:** Core logic in `src/lib.rs` and CLI entry point in `src/main.rs`.
16-
- **Shared code:** Parser modules (`src/parser/`) are used by both library and CLI (`parse_frames`, `parse_headers_from_text`).
17-
- **Duplicated code:** Export implementations exist separately in `src/export.rs` (library) and `src/main.rs` (CLI). CLI does NOT use library export functions.
18-
- **Current state:** **Partial unification** — parsing shared, export duplicated (~1800 lines in `src/main.rs`).
16+
- **Shared code:** Parser modules (`src/parser/`) and export functions (`src/export.rs`) are shared by both library and CLI.
17+
- **CLI as thin wrapper:** The CLI (`src/main.rs`) uses library export functions (`export_to_csv`, `export_to_gpx`, `export_to_event`) with CLI-specific status messages.
18+
- **Current state:** **Full unification complete** — parsing and export layers unified, CLI reduced from ~1800 to ~1400 lines.
1919
- **Decision criteria:** "Is this needed by crate consumers?" determines placement — shared logic in library, CLI-only logic in `src/main.rs`.
2020
- **Feature flags:** `csv`, `cli`, `json`, `serde` control optional dependencies; default: `csv` + `cli`.
2121
- **CRATE_USAGE.md reference:** See `CRATE_USAGE.md` for library API examples with feature flags.
22-
- **Code quality goals:** Reduce duplication by migrating CLI export logic to use library `export_to_csv()`, `export_to_gpx()`, `export_to_event()` functions.
2322
- **Testing:** Comprehensive tests distributed across `src/main.rs`, `src/conversion.rs`, `src/parser/stream.rs`, and `src/parser/helpers.rs`.
24-
- **Public API:** `parse_bbl_file()`, `parse_bbl_bytes()`, `BBLLog`, `ExportOptions`, conversion utilities, parser helpers.
23+
- **Public API:** `parse_bbl_file()`, `parse_bbl_bytes()`, `BBLLog`, `ExportOptions`, `export_to_csv()`, `export_to_gpx()`, `export_to_event()`, conversion utilities, parser helpers.
2524

2625
## Algorithms
2726
- **Method Selection:**

examples/export_demo.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ fn main() -> Result<()> {
108108
&log.gps_coordinates,
109109
&log.home_coordinates,
110110
&export_opts,
111+
log.header.log_start_datetime.as_deref(),
111112
)?;
112113
println!("✓ GPX export complete");
113114
} else {

examples/gpx_export.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ fn main() -> anyhow::Result<()> {
4747
&log.gps_coordinates,
4848
&log.home_coordinates,
4949
&export_opts,
50+
log.header.log_start_datetime.as_deref(),
5051
)?;
5152
println!("✓ GPX export complete");
5253
println!(" Exported {} GPS coordinates", log.gps_coordinates.len());

examples/multi_export.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ fn main() -> anyhow::Result<()> {
8383
&log.gps_coordinates,
8484
&log.home_coordinates,
8585
&export_opts,
86+
log.header.log_start_datetime.as_deref(),
8687
)?;
8788
println!(
8889
"✓ GPX export complete ({} coordinates)",

src/export.rs

Lines changed: 80 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,63 @@ pub struct ExportOptions {
2525
pub force_export: bool,
2626
}
2727

28+
/// Extract the base filename from an input path with consistent fallback.
29+
/// Used by all export functions and path computation helpers to ensure
30+
/// consistent naming across CSV, GPX, and event exports.
31+
///
32+
/// Always returns "blackbox" as fallback for missing or non-UTF-8 filenames,
33+
/// ensuring compute_export_paths() predictions match actual export filenames.
34+
fn extract_base_name(input_path: &Path) -> &str {
35+
input_path
36+
.file_stem()
37+
.and_then(|s| s.to_str())
38+
.unwrap_or("blackbox")
39+
}
40+
41+
/// Helper to compute export file paths with consistent naming across all export types.
42+
/// Ensures CLI status messages match actual filenames written by export functions.
43+
///
44+
/// # Arguments
45+
/// * `input_path` - Path to the input BBL file (used to extract base filename)
46+
/// * `export_options` - Export configuration with optional output directory
47+
/// * `log_number` - 1-based log number (for .NN suffix when multiple logs)
48+
/// * `total_logs` - Total number of logs in the file
49+
///
50+
/// # Returns
51+
/// Tuple of (csv_path, headers_path, gpx_path, event_path) using consistent naming
52+
pub fn compute_export_paths(
53+
input_path: &Path,
54+
export_options: &ExportOptions,
55+
log_number: usize,
56+
total_logs: usize,
57+
) -> (
58+
std::path::PathBuf,
59+
std::path::PathBuf,
60+
std::path::PathBuf,
61+
std::path::PathBuf,
62+
) {
63+
let base_name = extract_base_name(input_path);
64+
65+
let output_dir = if let Some(ref dir) = export_options.output_dir {
66+
std::path::Path::new(dir)
67+
} else {
68+
input_path.parent().unwrap_or(std::path::Path::new("."))
69+
};
70+
71+
let log_suffix = if total_logs > 1 {
72+
format!(".{:02}", log_number)
73+
} else {
74+
String::new()
75+
};
76+
77+
let csv_path = output_dir.join(format!("{}{}.csv", base_name, log_suffix));
78+
let headers_path = output_dir.join(format!("{}{}.headers.csv", base_name, log_suffix));
79+
let gpx_path = output_dir.join(format!("{}{}.gps.gpx", base_name, log_suffix));
80+
let event_path = output_dir.join(format!("{}{}.event", base_name, log_suffix));
81+
82+
(csv_path, headers_path, gpx_path, event_path)
83+
}
84+
2885
/// Pre-computed CSV field mapping for performance
2986
#[derive(Debug)]
3087
struct CsvFieldMap {
@@ -87,10 +144,7 @@ pub fn export_to_csv(
87144
input_path: &Path,
88145
export_options: &ExportOptions,
89146
) -> Result<()> {
90-
let base_name = input_path
91-
.file_stem()
92-
.and_then(|s| s.to_str())
93-
.unwrap_or("blackbox");
147+
let base_name = extract_base_name(input_path);
94148

95149
let output_dir = if let Some(ref dir) = export_options.output_dir {
96150
Path::new(dir)
@@ -288,22 +342,34 @@ fn export_flight_data_to_csv(log: &BBLLog, output_path: &Path) -> Result<()> {
288342
}
289343

290344
/// Export GPS data to GPX format
345+
///
346+
/// # Arguments
347+
/// * `input_path` - Path to the input BBL file (used for output naming)
348+
/// * `log_index` - Index of the current log (0-based)
349+
/// * `total_logs` - Total number of logs in the file
350+
/// * `gps_coordinates` - GPS coordinate data to export
351+
/// * `_home_coordinates` - Home coordinates (reserved for future use)
352+
/// * `export_options` - Export configuration options
353+
/// * `log_start_datetime` - Optional log start datetime from header for accurate timestamps
354+
///
355+
/// # Performance Notes
356+
/// For very large GPS traces, the `log_start_datetime` is parsed via `generate_gpx_timestamp()`
357+
/// on each trackpoint. Future optimization: consider caching the parsed base epoch once per log
358+
/// to avoid repeated parsing overhead when exporting thousands of GPS points.
291359
pub fn export_to_gpx(
292360
input_path: &Path,
293361
log_index: usize,
294362
total_logs: usize,
295363
gps_coordinates: &[GpsCoordinate],
296364
_home_coordinates: &[GpsHomeCoordinate],
297365
export_options: &ExportOptions,
366+
log_start_datetime: Option<&str>,
298367
) -> Result<()> {
299368
if gps_coordinates.is_empty() {
300369
return Ok(());
301370
}
302371

303-
let base_name = input_path
304-
.file_stem()
305-
.and_then(|n| n.to_str())
306-
.unwrap_or("unknown");
372+
let base_name = extract_base_name(input_path);
307373

308374
let output_dir = export_options
309375
.output_dir
@@ -339,19 +405,14 @@ pub fn export_to_gpx(
339405
}
340406
}
341407

342-
// Convert timestamp to ISO format
343-
let total_seconds = coord.timestamp_us / 1_000_000;
344-
let microseconds = coord.timestamp_us % 1_000_000;
345-
346-
// Use March 26, 2025 as base date
347-
let hours = 5 + (total_seconds / 3600) % 24;
348-
let minutes = (total_seconds % 3600) / 60;
349-
let seconds = total_seconds % 60;
408+
// Generate GPX timestamp from log_start_datetime + frame timestamp
409+
// Following blackbox_decode approach: dateTime + (gpsFrameTime / 1000000)
410+
let timestamp_str = generate_gpx_timestamp(log_start_datetime, coord.timestamp_us);
350411

351412
writeln!(
352413
gpx_file,
353-
r#" <trkpt lat="{:.7}" lon="{:.7}"><ele>{:.2}</ele><time>2025-03-26T{:02}:{:02}:{:02}.{:06}Z</time></trkpt>"#,
354-
coord.latitude, coord.longitude, coord.altitude, hours, minutes, seconds, microseconds
414+
r#" <trkpt lat="{:.7}" lon="{:.7}"><ele>{:.2}</ele><time>{}</time></trkpt>"#,
415+
coord.latitude, coord.longitude, coord.altitude, timestamp_str
355416
)?;
356417
}
357418

@@ -373,10 +434,7 @@ pub fn export_to_event(
373434
return Ok(());
374435
}
375436

376-
let base_name = input_path
377-
.file_stem()
378-
.and_then(|n| n.to_str())
379-
.unwrap_or("unknown");
437+
let base_name = extract_base_name(input_path);
380438

381439
let output_dir = export_options
382440
.output_dir

0 commit comments

Comments
 (0)