From b8302b8dbe96ba53d4affc1d1e48893807f045dd Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 29 Sep 2026 20:16:40 +0000 Subject: [PATCH 1/2] Let email.always send every Run's notification, and --no-email skip it email.always = true in the User config makes a Run without --email send a Run notification to email.to, through the same preflight as the flag. no-email / --no-email skips it for one Run, and contradicts --email. Co-Authored-By: Claude Opus 5.5 --- README.md | 11 +++-- src/args.rs | 32 +++++++++++--- src/config.rs | 48 ++++++++++++++++++++ src/main.rs | 8 +++- tests/command_surface.rs | 24 ++++++++++ tests/run_notification.rs | 92 +++++++++++++++++++++++++++++++++++++++ 6 files changed, 202 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 265408279..4a7bdabea 100644 --- a/README.md +++ b/README.md @@ -104,7 +104,7 @@ To have the Run merge its pull request instead of leaving it for your review, st thirdshift merge https://github.com/acme/widgets/issues/7 ``` -`thirdshift --merge ` does the same, and either goes before or after the URL. The agent gets the same implement prompt either way; the differences are the Self-merge at the end and the review of [Foreign commits](#foreign-commits-in-a-merge-run). To make every Run on a machine a Merge run, set `merge.always` in the [User config](#user-config); `no-merge` (or `--no-merge`), before or after the URL, then makes one Run end ready for review instead. Giving a flag twice, or `merge` together with `no-merge`, is an argument error. +`thirdshift --merge ` does the same, and either goes before or after the URL. The agent gets the same implement prompt either way; the differences are the Self-merge at the end and the review of [Foreign commits](#foreign-commits-in-a-merge-run). To make every Run on a machine a Merge run, set `merge.always` in the [User config](#user-config); `no-merge` (or `--no-merge`), before or after the URL, then makes one Run end ready for review instead. Giving a flag twice, `merge` together with `no-merge`, or `email` together with `no-email`, is an argument error. A Merge run ends in one of three ways: @@ -118,7 +118,7 @@ To be emailed when the Run ends, add `--email` (or `email`), optionally followed thirdshift --email you@example.com https://github.com/acme/widgets/issues/7 ``` -It sends one [Run notification](#run-notifications), whatever the outcome. +It sends one [Run notification](#run-notifications), whatever the outcome. To have every Run on a machine send one, set `email.always` in the [User config](#user-config); `no-email` (or `--no-email`) then skips it for one Run. - **stdout** carries only the pull request's URL: on success, and on a Failed run that leaves an open pull request, a draft or, after a policy refusal, one ready for review. The exit code tells the two apart, so script it as `url=$(thirdshift "$issue") && echo "ready: $url"`. - **stderr** carries everything else: errors, cleanup problems, and progress lines while sessions run. A successful Run's last line names the pull request too: `PR is ready for review`, or `PR is merged` after a Merge run, followed only by a `warning:` line if a [Run notification](#run-notifications) can't be sent. @@ -146,6 +146,7 @@ A **User config** at `~/.thirdshift/config.toml` sets this machine's defaults fo always = true # every Run is a Merge run, without the merge word [email] +always = true # every Run sends a Run notification, without the email word to = "you@example.com" # where email goes when the command names no address from = "thirdshift@your-verified-domain.com" # the sender; onboarding@resend.dev if unset @@ -155,6 +156,8 @@ dir = "~/elsewhere/logs" # where session logs go, instead of ~/.thirdshift/log With `merge.always = true`, `thirdshift ` is a Merge run, and `thirdshift --no-merge ` (or `no-merge`, before or after the URL) leaves that one Run's pull request ready for review. +With `email.always = true`, every Run sends a [Run notification](#run-notifications) to `email.to`, as if given `--email`, and `thirdshift --no-email ` (or `no-email`, before or after the URL) sends none for that one Run. + `logs.dir` sets the directory [session logs](#logs) are written to, created if missing. It must be an absolute path, `~` or a path starting with `~/`, where `~` stands for `$HOME`. A relative path stops the Run before any work, since the directory a Run is launched from is no base for a setting that holds for every Run. A Run reads the file before any work. One that isn't valid TOML, or that has a key or section thirdshift doesn't know, such as `alway` for `always`, stops the Run with an error naming the file and the offending key, so a typo can't silently leave a setting off. `email-test` reads it the same way. `update`, `version` and `help` never read it, so a broken User config can't block them. @@ -178,9 +181,9 @@ It sends one test email, whose subject marks it as a test and whose body names t #### Run notifications -`--email` (or `email`) asks a Run for a **Run notification**: one email, sent when the Run ends, whatever the outcome. The word after the flag is the address only if it contains `@` and doesn't start with `https://`, so the Issue URL is never taken for it; otherwise the email goes to `email.to`. Giving the flag twice is an argument error. +`--email` (or `email`) asks a Run for a **Run notification**: one email, sent when the Run ends, whatever the outcome. The word after the flag is the address only if it contains `@` and doesn't start with `https://`, so the Issue URL is never taken for it; otherwise the email goes to `email.to`. With `email.always = true` in the [User config](#user-config), a Run asks for one without the flag, and `--no-email` (or `no-email`) skips it for that Run; an address after `--email` still wins over `email.to`. Giving a flag twice, or `--email` together with `--no-email`, is an argument error. -The Run makes the same checks as `email-test` before any other work: an address is known, and `RESEND_API_KEY` is set and not empty. If either fails, the Run stops, exits `1` naming what's missing, and sends nothing. Once they pass, every way the Run ends sends exactly one notification, after its outcome is final and its cleanup done: ready for review, merged, a [Failed run](#failed-runs) (including a later preflight failure such as an origin mismatch), or interrupted by Ctrl-C, SIGTERM or a closed terminal. +A Run that asks for a notification, by the flag or by `email.always`, makes the same checks as `email-test` before any other work: an address is known, and `RESEND_API_KEY` is set and not empty. If either fails, the Run stops, exits `1` naming what's missing, and sends nothing. Once they pass, every way the Run ends sends exactly one notification, after its outcome is final and its cleanup done: ready for review, merged, a [Failed run](#failed-runs) (including a later preflight failure such as an origin mismatch), or interrupted by Ctrl-C, SIGTERM or a closed terminal. - **Subject**: `[thirdshift] /# : `, where the outcome is `ready for review`, `merged`, `failed` or `interrupted`. The title is left out if it can't be read from GitHub. - **Body**, plain text: the pull request URL (if any), the failure cause (if failed), the session log path (if any), the hostname and how long the Run took. diff --git a/src/args.rs b/src/args.rs index ea1a39b6d..7c5dd140b 100644 --- a/src/args.rs +++ b/src/args.rs @@ -21,15 +21,25 @@ pub struct RunArgs { /// The goal `merge` or `no-merge` asked for, if either was given; without /// one, the User config decides. pub goal: Option, - /// `Some` if `email` was given, so the Run sends a Run notification, - /// holding the address after it, if any. - pub email: Option>, + /// What `email` or `no-email` asked for, if either was given; without + /// one, the User config decides. + pub email: Option, +} + +/// What a Run's command asked about its Run notification. +#[derive(Debug, PartialEq, Eq)] +pub enum Email { + /// `email`: send one, to the address after it, if any. + Send(Option), + /// `no-email`: send none. + Skip, } /// Parse the arguments after the program name. `help`, `version`, `update` /// and `email-test` are commands only as the first argument. Otherwise it is /// a Run: one Issue URL, with each Run flag at most once, before or after it. /// `email` may be followed by the address to send the Run notification to. +/// `merge` and `no-merge` contradict each other, as do `email` and `no-email`. pub fn parse(args: &[String]) -> Result { match args.first().map(String::as_str) { Some("help" | "--help" | "-h") => return Ok(Command::Help), @@ -52,11 +62,19 @@ pub fn parse(args: &[String]) -> Result { let asked = match arg.as_str() { "merge" | "--merge" => Goal::Merged, "no-merge" | "--no-merge" => Goal::ReadyForReview, - "email" | "--email" => { - if email.is_some() { - bail!("repeated argument: {arg}"); + "email" | "--email" | "no-email" | "--no-email" => { + let asked = if arg.ends_with("no-email") { + Email::Skip + } else { + Email::Send(args.next_if(|next| is_address(next)).cloned()) + }; + match (&email, &asked) { + (None, _) => email = Some(asked), + (Some(Email::Skip), Email::Skip) | (Some(Email::Send(_)), Email::Send(_)) => { + bail!("repeated argument: {arg}") + } + (Some(_), _) => bail!("email and no-email can't be used together"), } - email = Some(args.next_if(|next| is_address(next)).cloned()); continue; } _ => { diff --git a/src/config.rs b/src/config.rs index 09c8eb239..56c0d7de8 100644 --- a/src/config.rs +++ b/src/config.rs @@ -7,6 +7,7 @@ use std::path::{Path, PathBuf}; use anyhow::{Context, Result, anyhow, bail}; use toml::{Table, Value}; +use crate::args::Email; use crate::run::Goal; /// The settings a User config can hold. Each is what a Run does when its @@ -27,6 +28,9 @@ pub struct UserConfig { /// secret. #[derive(Debug, Default, PartialEq, Eq)] pub struct EmailSettings { + /// `email.always`: every Run sends a Run notification unless told + /// `no-email`. + pub always: bool, /// `email.to`: the address email goes to when the command names none. pub to: Option, /// `email.from`: the sender, in place of Resend's shared test sender. @@ -88,6 +92,8 @@ impl UserConfig { ), }, ("logs", "dir", _) => bail!("logs.dir must be a string in {file}"), + ("email", "always", Value::Boolean(always)) => config.email.always = *always, + ("email", "always", _) => bail!("email.always must be true or false in {file}"), ("email", "to", Value::String(to)) => config.email.to = Some(to.clone()), ("email", "from", Value::String(from)) => { config.email.from = Some(from.clone()) @@ -112,6 +118,19 @@ impl UserConfig { } } +impl EmailSettings { + /// Whether a Run whose command asked `asked` about email sends a Run + /// notification: `Some` if it does, holding the address the command + /// gave, if any. + pub fn notification(&self, asked: Option) -> Option> { + match asked { + Some(Email::Send(to)) => Some(to), + Some(Email::Skip) => None, + None => self.always.then_some(None), + } + } +} + /// `path` as an absolute path, with a leading `~` expanded to `home`, or /// `None` if it is relative: the directory a Run is launched from is no base /// for a setting that holds for every Run. @@ -149,6 +168,31 @@ mod tests { assert_eq!(config.email.to.as_deref(), Some("me@example.com")); assert_eq!(config.email.from.as_deref(), Some("ts@acme.dev")); assert_eq!(parse("[email]\n").unwrap().email, EmailSettings::default()); + assert!(parse("[email]\nalways = true\n").unwrap().email.always); + assert!(!parse("[email]\nalways = false\n").unwrap().email.always); + } + + #[test] + fn the_command_decides_the_notification_before_email_always() { + for always in [false, true] { + let settings = EmailSettings { + always, + ..EmailSettings::default() + }; + let to = Some("me@example.com".to_string()); + assert_eq!( + settings.notification(Some(Email::Send(to.clone()))), + Some(to) + ); + assert_eq!(settings.notification(Some(Email::Send(None))), Some(None)); + assert_eq!(settings.notification(Some(Email::Skip)), None); + } + let always = EmailSettings { + always: true, + ..EmailSettings::default() + }; + assert_eq!(always.notification(None), Some(None)); + assert_eq!(EmailSettings::default().notification(None), None); } #[test] @@ -203,6 +247,10 @@ mod tests { ), ("[email]\nadress = \"a@b.c\"\n", "unknown key email.adress"), ("email = \"a@b.c\"\n", "email must be the section [email]"), + ( + "[email]\nalways = \"yes\"\n", + "email.always must be true or false", + ), ( "[email]\nto = 1\n", "email.to must be a quoted email address", diff --git a/src/main.rs b/src/main.rs index acf95d2a2..82818f9e1 100644 --- a/src/main.rs +++ b/src/main.rs @@ -41,7 +41,7 @@ usage: thirdshift Run the factory on the issue, from th thirdshift version Print thirdshift's version thirdshift help Print this help -merge, --no-merge and --email go before or after the Issue URL, in any order. +merge, --no-merge, --email and --no-email go before or after the Issue URL, in any order. --email sends one Run notification when the Run ends, whatever the outcome: ready for review, merged, failed or interrupted. --email
sends it to
; a word @@ -60,9 +60,11 @@ logs.dir sets where session logs go instead of ~/.thirdshift/logs: an absolute p --email and email-test send to their
, else to email.to, from email.from, else from onboarding@resend.dev, which only delivers to your own Resend account's address. +With email.always set, every Run sends a Run notification unless given --no-email. The Resend API key comes only from the RESEND_API_KEY environment variable: [email] + always = true to = \"you@example.com\" from = \"thirdshift@your-verified-domain.com\" "; @@ -100,7 +102,9 @@ fn main() -> ExitCode { progress::step(format_args!("{error:#}")); return ExitCode::FAILURE; } - let notification = match email + let notification = match config + .email + .notification(email) .map(|to| RunNotification::new(to, &config.email, &issue)) .transpose() { diff --git a/tests/command_surface.rs b/tests/command_surface.rs index 86928ad7c..1130a76cb 100644 --- a/tests/command_surface.rs +++ b/tests/command_surface.rs @@ -170,6 +170,7 @@ fn a_repeated_flag_prints_an_error_and_the_help_to_stderr() { (vec!["--merge", url.as_str(), "--merge"], "--merge"), (vec!["merge", "--merge", url.as_str()], "--merge"), (vec![url.as_str(), "no-merge", "--no-merge"], "--no-merge"), + (vec!["no-email", url.as_str(), "--no-email"], "--no-email"), (vec!["--email", url.as_str(), "--email"], "--email"), ( vec!["email", "a@example.com", url.as_str(), "email"], @@ -212,6 +213,27 @@ fn merge_with_no_merge_prints_an_error_and_the_help_to_stderr() { } } +#[test] +fn email_with_no_email_prints_an_error_and_the_help_to_stderr() { + let scenario = Scenario::new(); + let url = scenario.issue_url(7); + + for args in [ + vec!["email", url.as_str(), "no-email"], + vec!["--no-email", "--email", "a@example.com", url.as_str()], + vec![url.as_str(), "--email", "--no-email"], + ] { + let result = scenario.run(&args); + + assert_argument_error( + &scenario, + &result, + "email and no-email can't be used together", + ); + assert!(scenario.claude_calls().is_empty(), "{args:?} started a Run"); + } +} + #[test] fn no_merge_without_an_issue_url_prints_an_error_and_the_help_to_stderr() { let scenario = Scenario::new(); @@ -245,6 +267,8 @@ fn help_lists_the_email_settings() { "email.from", "Run notification", "--email
", + "email.always", + "--no-email", "onboarding@resend.dev", "RESEND_API_KEY", ] { diff --git a/tests/run_notification.rs b/tests/run_notification.rs index 79291667b..f35f10b1a 100644 --- a/tests/run_notification.rs +++ b/tests/run_notification.rs @@ -338,3 +338,95 @@ fn a_failed_send_is_a_warning_that_changes_neither_the_exit_code_nor_stdout() { } } } + +/// The User config of a machine where every Run sends a Run notification. +const EMAIL_ALWAYS: &str = "[email]\nalways = true\nto = \"config@example.com\"\n"; + +#[test] +fn email_always_makes_a_run_without_the_flag_send_one_notification_to_email_to() { + let scenario = Scenario::new(); + scenario.user_config_is(EMAIL_ALWAYS); + scenario.agent_does(AGENT_OPENS_PR); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run(&scenario, &resend, &[&scenario.issue_url(7)], Some(KEY)); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let request = the_one_request(&resend); + assert_eq!(request.body["to"], "config@example.com"); + assert!( + subject(&request).ends_with(": ready for review"), + "{}", + subject(&request) + ); +} + +#[test] +fn no_email_skips_the_notification_email_always_asks_for() { + for no_email in ["no-email", "--no-email"] { + let scenario = Scenario::new(); + scenario.user_config_is(EMAIL_ALWAYS); + scenario.agent_does(AGENT_OPENS_PR); + let resend = ResendStandIn::replying(200, ACCEPTED); + let url = scenario.issue_url(7); + + let result = run(&scenario, &resend, &[no_email, &url], Some(KEY)); + + assert_eq!(result.code, Some(0), "{no_email}: {}", result.stderr); + assert!(resend.requests().is_empty(), "{no_email}"); + } +} + +#[test] +fn with_email_always_an_address_after_the_flag_still_wins() { + let scenario = Scenario::new(); + scenario.user_config_is(EMAIL_ALWAYS); + scenario.agent_does(AGENT_OPENS_PR); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run( + &scenario, + &resend, + &[&scenario.issue_url(7), "--email", "flag@example.com"], + Some(KEY), + ); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(the_one_request(&resend).body["to"], "flag@example.com"); +} + +#[test] +fn email_always_without_an_address_or_a_key_stops_the_run_before_any_work() { + for (config, key, named) in [ + ("[email]\nalways = true\n", Some(KEY), "no email address"), + (EMAIL_ALWAYS, None, "RESEND_API_KEY"), + ] { + let scenario = Scenario::new(); + scenario.user_config_is(config); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run(&scenario, &resend, &[&scenario.issue_url(7)], key); + + scenario.assert_rejected_before_any_work(&result, named); + assert!(scenario.gh_calls().is_empty(), "{:?}", scenario.gh_calls()); + assert!(resend.requests().is_empty()); + } +} + +#[test] +fn without_email_always_a_run_without_the_flag_sends_nothing() { + for config in [ + "[email]\nalways = false\nto = \"me@example.com\"\n", + "[email]\nto = \"me@example.com\"\n", + ] { + let scenario = Scenario::new(); + scenario.user_config_is(config); + scenario.agent_does(AGENT_OPENS_PR); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run(&scenario, &resend, &[&scenario.issue_url(7)], Some(KEY)); + + assert_eq!(result.code, Some(0), "{config}: {}", result.stderr); + assert!(resend.requests().is_empty(), "{config}"); + } +} From 71b31f0e9bb8276aeb9508f63d0c1499a0df3bb6 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 29 Sep 2026 20:21:30 +0000 Subject: [PATCH 2/2] Resolve the notification ask like the goal, with one flag-once rule NotificationAsk lives with RunNotification, so the User config no longer depends on argument parsing, and email.always gives a default ask as merge.always gives a default goal. The merge and email flags share one check for a flag repeated or contradicted. Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- src/args.rs | 65 ++++++++++++++++++++++----------------------- src/config.rs | 42 +++++++++++------------------ src/main.rs | 13 +++++---- src/notification.rs | 10 +++++++ 5 files changed, 65 insertions(+), 67 deletions(-) diff --git a/README.md b/README.md index 4a7bdabea..3ae25415a 100644 --- a/README.md +++ b/README.md @@ -104,7 +104,7 @@ To have the Run merge its pull request instead of leaving it for your review, st thirdshift merge https://github.com/acme/widgets/issues/7 ``` -`thirdshift --merge ` does the same, and either goes before or after the URL. The agent gets the same implement prompt either way; the differences are the Self-merge at the end and the review of [Foreign commits](#foreign-commits-in-a-merge-run). To make every Run on a machine a Merge run, set `merge.always` in the [User config](#user-config); `no-merge` (or `--no-merge`), before or after the URL, then makes one Run end ready for review instead. Giving a flag twice, `merge` together with `no-merge`, or `email` together with `no-email`, is an argument error. +`thirdshift --merge ` does the same, and either goes before or after the URL. The agent gets the same implement prompt either way; the differences are the Self-merge at the end and the review of [Foreign commits](#foreign-commits-in-a-merge-run). To make every Run on a machine a Merge run, set `merge.always` in the [User config](#user-config); `no-merge` (or `--no-merge`), before or after the URL, then makes one Run end ready for review instead. Giving a flag twice, or `merge` together with `no-merge`, is an argument error. A Merge run ends in one of three ways: diff --git a/src/args.rs b/src/args.rs index 7c5dd140b..6ecb7fd71 100644 --- a/src/args.rs +++ b/src/args.rs @@ -1,8 +1,11 @@ //! The command line: which command, and for a Run, its Issue URL and flags. +use std::mem::discriminant; + use anyhow::{Result, bail}; use crate::issue::IssueUrl; +use crate::notification::NotificationAsk; use crate::run::Goal; /// What thirdshift was asked to do. @@ -23,16 +26,7 @@ pub struct RunArgs { pub goal: Option, /// What `email` or `no-email` asked for, if either was given; without /// one, the User config decides. - pub email: Option, -} - -/// What a Run's command asked about its Run notification. -#[derive(Debug, PartialEq, Eq)] -pub enum Email { - /// `email`: send one, to the address after it, if any. - Send(Option), - /// `no-email`: send none. - Skip, + pub email: Option, } /// Parse the arguments after the program name. `help`, `version`, `update` @@ -59,36 +53,24 @@ pub fn parse(args: &[String]) -> Result { let mut email = None; let mut args = args.iter().peekable(); while let Some(arg) = args.next() { - let asked = match arg.as_str() { - "merge" | "--merge" => Goal::Merged, - "no-merge" | "--no-merge" => Goal::ReadyForReview, - "email" | "--email" | "no-email" | "--no-email" => { - let asked = if arg.ends_with("no-email") { - Email::Skip - } else { - Email::Send(args.next_if(|next| is_address(next)).cloned()) - }; - match (&email, &asked) { - (None, _) => email = Some(asked), - (Some(Email::Skip), Email::Skip) | (Some(Email::Send(_)), Email::Send(_)) => { - bail!("repeated argument: {arg}") - } - (Some(_), _) => bail!("email and no-email can't be used together"), - } - continue; + match arg.as_str() { + "merge" | "--merge" => ask_once(&mut goal, Goal::Merged, arg, MERGE_FLAGS)?, + "no-merge" | "--no-merge" => { + ask_once(&mut goal, Goal::ReadyForReview, arg, MERGE_FLAGS)? + } + "email" | "--email" => { + let to = args.next_if(|next| is_address(next)).cloned(); + ask_once(&mut email, NotificationAsk::Send(to), arg, EMAIL_FLAGS)? + } + "no-email" | "--no-email" => { + ask_once(&mut email, NotificationAsk::Skip, arg, EMAIL_FLAGS)? } _ => { if issue.is_some() { bail!("unexpected argument after the Issue URL: {arg}"); } issue = Some(IssueUrl::parse(arg)?); - continue; } - }; - match goal { - None => goal = Some(asked), - Some(given) if given == asked => bail!("repeated argument: {arg}"), - Some(_) => bail!("merge and no-merge can't be used together"), } } let Some(issue) = issue else { @@ -97,6 +79,23 @@ pub fn parse(args: &[String]) -> Result { Ok(Command::Run(RunArgs { issue, goal, email })) } +const MERGE_FLAGS: &str = "merge and no-merge"; +const EMAIL_FLAGS: &str = "email and no-email"; + +/// Record in `given` what the flag `arg` asked for: the same kind of ask +/// twice is a repeated argument, and a different one contradicts the first, +/// as one of the pair of `flags` can't go with the other. +fn ask_once(given: &mut Option, asked: T, arg: &str, flags: &str) -> Result<()> { + match given { + None => *given = Some(asked), + Some(given) if discriminant(given) == discriminant(&asked) => { + bail!("repeated argument: {arg}") + } + Some(_) => bail!("{flags} can't be used together"), + } + Ok(()) +} + /// Is `arg`, after `email`, the address to send to? Only if it looks like /// one, so the Issue URL is never taken for it. fn is_address(arg: &str) -> bool { diff --git a/src/config.rs b/src/config.rs index 56c0d7de8..874dd3960 100644 --- a/src/config.rs +++ b/src/config.rs @@ -7,7 +7,7 @@ use std::path::{Path, PathBuf}; use anyhow::{Context, Result, anyhow, bail}; use toml::{Table, Value}; -use crate::args::Email; +use crate::notification::NotificationAsk; use crate::run::Goal; /// The settings a User config can hold. Each is what a Run does when its @@ -119,14 +119,13 @@ impl UserConfig { } impl EmailSettings { - /// Whether a Run whose command asked `asked` about email sends a Run - /// notification: `Some` if it does, holding the address the command - /// gave, if any. - pub fn notification(&self, asked: Option) -> Option> { - match asked { - Some(Email::Send(to)) => Some(to), - Some(Email::Skip) => None, - None => self.always.then_some(None), + /// What a Run whose command gave no `email` or `no-email` asks about its + /// Run notification. + pub fn default_ask(&self) -> NotificationAsk { + if self.always { + NotificationAsk::Send(None) + } else { + NotificationAsk::Skip } } } @@ -173,26 +172,17 @@ mod tests { } #[test] - fn the_command_decides_the_notification_before_email_always() { - for always in [false, true] { - let settings = EmailSettings { - always, - ..EmailSettings::default() - }; - let to = Some("me@example.com".to_string()); + fn email_always_asks_for_a_notification_to_email_to() { + let always = parse("[email]\nalways = true\n").unwrap(); + assert_eq!(always.email.default_ask(), NotificationAsk::Send(None)); + for text in ["", "[email]\nalways = false\n"] { + let config = parse(text).unwrap(); assert_eq!( - settings.notification(Some(Email::Send(to.clone()))), - Some(to) + config.email.default_ask(), + NotificationAsk::Skip, + "{text:?}" ); - assert_eq!(settings.notification(Some(Email::Send(None))), Some(None)); - assert_eq!(settings.notification(Some(Email::Skip)), None); } - let always = EmailSettings { - always: true, - ..EmailSettings::default() - }; - assert_eq!(always.notification(None), Some(None)); - assert_eq!(EmailSettings::default().notification(None), None); } #[test] diff --git a/src/main.rs b/src/main.rs index 82818f9e1..5dcd46098 100644 --- a/src/main.rs +++ b/src/main.rs @@ -27,7 +27,7 @@ use std::process::ExitCode; use args::{Command, RunArgs}; use config::UserConfig; -use notification::RunNotification; +use notification::{NotificationAsk, RunNotification}; const HELP: &str = "\ thirdshift turns a GitHub issue into a ready-for-review pull request, or a merged one, unattended. @@ -102,12 +102,11 @@ fn main() -> ExitCode { progress::step(format_args!("{error:#}")); return ExitCode::FAILURE; } - let notification = match config - .email - .notification(email) - .map(|to| RunNotification::new(to, &config.email, &issue)) - .transpose() - { + let notification = match email.unwrap_or(config.email.default_ask()) { + NotificationAsk::Send(to) => RunNotification::new(to, &config.email, &issue).map(Some), + NotificationAsk::Skip => Ok(None), + }; + let notification = match notification { Ok(notification) => notification, Err(error) => { progress::step(format_args!("{error:#}")); diff --git a/src/notification.rs b/src/notification.rs index a0df9187e..d99f6671c 100644 --- a/src/notification.rs +++ b/src/notification.rs @@ -15,6 +15,16 @@ use crate::issue::IssueUrl; use crate::progress; use crate::run::{Goal, Reached}; +/// What a Run asks about its Run notification, by its command or, without +/// `email` or `no-email`, by the User config. +#[derive(Debug, PartialEq, Eq)] +pub enum NotificationAsk { + /// Send one, to this address, else to `email.to`. + Send(Option), + /// Send none. + Skip, +} + /// A Run notification, checked and waiting for the Run to end. pub struct RunNotification { resend: Resend,