From 4a1aab105b3d4f1cc2fff6aafb56f30f955c6fc6 Mon Sep 17 00:00:00 2001
From: Michael Diamond <dimo414@gmail.com>
Date: Sat, 20 Jan 2024 13:53:09 -0800
Subject: [PATCH] Make --ttl a required argument

This is a breaking change but the default behavior is arbitrary and it's
unlikely most users actually need or want this TTL. Furthermore, making
this change will enable introducing other expiration mechanisms in the
future. More details in #48 and #27.
---
 src/main.rs  | 17 +++++++++++++++--
 tests/cli.rs | 10 ++++++----
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/src/main.rs b/src/main.rs
index d8cbfc5..88e2284 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -5,6 +5,7 @@ use std::process::{Command, exit, Stdio};
 use std::time::Duration;
 
 use anyhow::{Context, Result};
+use clap::error::{ContextKind, ContextValue, ErrorKind};
 use clap::Parser;
 
 use bkt::{CommandDesc, Bkt};
@@ -136,7 +137,7 @@ struct Cli {
     command: Vec<OsString>,
 
     /// Duration the cached result will be valid for
-    #[arg(long, value_name = "DURATION", default_value = "60s", visible_alias = "time-to-live", env = "BKT_TTL")]
+    #[arg(long, value_name = "DURATION", visible_alias = "time-to-live", env = "BKT_TTL")]
     ttl: humantime::Duration,
 
     /// Duration after which the result will be asynchronously refreshed
@@ -186,7 +187,19 @@ struct Cli {
 }
 
 fn main() {
-    let cli = Cli::parse();
+    // TODO remove this suggestion in 0.9.0
+    let mut cli = Cli::try_parse();
+    if let Err(err) = cli.as_mut() {
+        if matches!(err.kind(), ErrorKind::MissingRequiredArgument) {
+            // https://github.com/clap-rs/clap/discussions/5318
+            err.insert(ContextKind::Suggested, ContextValue::StyledStrs(vec![[
+                "Prior to 0.8.0 --ttl was optional, and defaulted to 60 seconds.",
+                "To preserve this behavior pass `--ttl=1m` or set `BKT_TTL=1m` in your environment."
+            ].join(" ").into()]));
+        }
+        err.exit();
+    }
+    let cli = cli.expect("Not Err");
 
     match run(cli) {
         Ok(code) => exit(code),
diff --git a/tests/cli.rs b/tests/cli.rs
index b51bf66..1577427 100644
--- a/tests/cli.rs
+++ b/tests/cli.rs
@@ -32,6 +32,8 @@ mod cli {
         }
         assert!(path.exists(), "Could not find bkt binary in {:?}", dir);
         let mut bkt = Command::new(&path);
+        // Set a TTL here rather than in every test - tests that care about the TTL should override
+        bkt.env("BKT_TTL", "5s");
         bkt.env("BKT_TMPDIR", cache_dir.as_ref().as_os_str());
         bkt
     }
@@ -116,7 +118,7 @@ mod cli {
     fn help() {
         let dir = TestDir::temp();
         let out = succeed(bkt(dir.path("cache")).arg("--help"));
-        assert!(out.contains("bkt [OPTIONS] -- <COMMAND>..."));
+        assert!(out.contains("bkt [OPTIONS] --ttl <DURATION> -- <COMMAND>..."), "Was:\n---\n{}\n---", out);
     }
 
     #[test]
@@ -142,16 +144,16 @@ mod cli {
         let dir = TestDir::temp();
         let file = dir.path("file");
         let args = ["--", "bash", "-c", COUNT_INVOCATIONS, "arg0", file.to_str().unwrap()];
-        let first_result = succeed(bkt(dir.path("cache")).args(args));
+        let first_result = succeed(bkt(dir.path("cache")).arg("--ttl=1m").args(args));
         assert_eq!(first_result, "1");
 
         // Slightly stale is still cached
         make_dir_stale(dir.path("cache"), Duration::from_secs(10)).unwrap();
-        let subsequent_result = succeed(bkt(dir.path("cache")).args(args));
+        let subsequent_result = succeed(bkt(dir.path("cache")).arg("--ttl=1m").args(args));
         assert_eq!(first_result, subsequent_result);
 
         make_dir_stale(dir.path("cache"), Duration::from_secs(120)).unwrap();
-        let after_stale_result = succeed(bkt(dir.path("cache")).args(args));
+        let after_stale_result = succeed(bkt(dir.path("cache")).arg("--ttl=1m").args(args));
         assert_eq!(after_stale_result, "2");
 
         // Respects BKT_TTL env var (other tests cover --ttl)