Skip to content

record: default output path is inconsistently documented and resolved (and not platform-aware) #223

Description

@inureyes

Summary

The record subcommand's default output path is documented inconsistently across --help, the source doc comments, and the README, and the effective path is resolved through a brittle string-sentinel mechanism. The cache base is also a hard-coded ~/.cache/... string that ignores platform conventions on Windows and macOS.

This was surfaced by #210 (@victorhooi), which correctly fixed the README table but left the deeper inconsistencies untouched. The CLA on that PR is unsigned, so the fix is re-implemented here, incorporating the README correction with credit to the original reporter.

Current behavior

  • Effective default (no -o, no config file): ~/.cache/all-smi/records/all-smi-record.ndjson.zst. Settings::default() always sets record.output_dir = "~/.cache/all-smi/records" (src/common/config_file.rs), and RecorderOptions::from_args_with_settings joins the default basename under it even when no config file is present (src/record/mod.rs).
  • all-smi record --help advertises the --output default as all-smi-record.ndjson.zst — i.e. the current working directory (src/cli.rs).
  • The source doc comment says the compiled default is ./all-smi-record.ndjson.zst (src/record/mod.rs).
  • The README (after Update default output path in README.md #210) is the only surface that states ~/.cache/....

Problems

  1. Docs disagreeall-smi record --help, where most users look first, contradicts the README and the actual behavior.
  2. Brittle resolution + latent wart — "operator did not pass -o" is detected by string-matching args.output against the magic basename (src/record/mod.rs); the in-code comment itself flags this as fragile ("if that ever changes, update this guard"). As a side effect, passing -o all-smi-record.ndjson.zst explicitly is silently redirected into the cache dir instead of the current directory.
  3. Not platform-aware~/.cache/... is a hard-coded string expanded via dirs::home_dir(), not the platform-aware dirs::cache_dir(). So on Windows it lands at C:\Users\<you>\.cache\all-smi\records\... (a Unix-style dotdir in the profile) and on macOS at ~/.cache/... rather than ~/Library/Caches/.... By contrast the config path uses dirs::config_dir() (%APPDATA%\all-smi on Windows), so the codebase is internally inconsistent.

Proposed solution

Scope A — core fix (low-risk, no change to where files land):

  • Make RecordArgs::output an Option<PathBuf> (drop the clap default_value); resolve the effective default explicitly in from_args_with_settings (None<output_dir>/<DEFAULT_BASENAME>, Some(p) → honored verbatim). This removes the string-sentinel fragility and the explicit--o redirect wart.
  • Introduce a single shared DEFAULT_BASENAME constant (dedupe the literal currently in src/cli.rs and src/record/mod.rs).
  • Align all doc surfaces on one default string: the clap long_help, the mod.rs doc comment, and the README (fold in Update default output path in README.md #210's README correction, credit @victorhooi).

Scope B — platform-correct cache location (decision needed; may be a separate issue):

  • B1: derive the cache base from dirs::cache_dir() so Windows → %LOCALAPPDATA%\all-smi\records, macOS → ~/Library/Caches/all-smi/records, Linux → $XDG_CACHE_HOME/all-smi/records. Apply consistently to the energy WAL and users CSV as well. Note: this moves existing files on macOS/Windows (and Linux when $XDG_CACHE_HOME is set) and requires a migration/changelog note.
  • B2: keep ~/.cache/... deliberately for cross-platform consistency, but document the rationale and the platform expansion explicitly.

Recommendation: land Scope A first; treat Scope B (especially B1, a behavior change) as a separate decision/PR to keep changes focused. Either way, the "config is platform-aware but cache is hard-coded ~/.cache" inconsistency is worth reconciling.

Affected files

  • Scope A: src/cli.rs, src/record/mod.rs, README.md
  • Scope B: src/common/config_file.rs, src/common/paths.rs, and for consistency src/metrics/energy_wal.rs, src/view/event_handler.rs

Acceptance criteria

  • all-smi record --help, the source doc comment, and the README all state the same default output path.
  • Default-path resolution no longer relies on string-matching the basename; an explicit -o <path> is always honored verbatim, including when it equals the default basename.
  • DEFAULT_BASENAME is defined once and shared between the CLI arg and the resolver.
  • (Scope B, if chosen) the cache location is platform-correct via dirs::cache_dir(), applied consistently across record output / energy WAL / users CSV, with a migration/changelog note.
  • Tests cover: no -o + no config; no -o + config output_dir set; explicit -o equal to the default basename; explicit -o absolute path.

References

Metadata

Metadata

Assignees

Labels

Type

No type

Fields

No fields configured for issues without a type.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions