From a59e99b107399c7c779aaeae392d98c7932d0b62 Mon Sep 17 00:00:00 2001 From: Valentin Haudiquet Date: Sat, 3 Oct 2026 01:48:27 +0200 Subject: [PATCH] new: type the scaffold validator errors The leaf validators of the wizard (source name, command, upstream version, maintainer, homepage, Depends, orig path, template id, file collisions) now return a shared options::ValidationError carrying the structured pieces (the rejected string, the wrapped DebianVersion or Deps parse failure) instead of pre-formatted Strings; the wizard resolve flow keeps its aggregate String errors and renders at the boundary, and the Prompter validator closures adapt with to_string() since the port's Validator deliberately exchanges re-ask explanations as plain strings. Display texts are unchanged, as are the tests that assert on them. --- src/new/options.rs | 236 ++++++++++++++++++++++++++++++------------- src/new/questions.rs | 45 +++++---- 2 files changed, 193 insertions(+), 88 deletions(-) diff --git a/src/new/options.rs b/src/new/options.rs index 12c9917..f40ad22 100644 --- a/src/new/options.rs +++ b/src/new/options.rs @@ -63,22 +63,12 @@ impl TemplateId { } /// Parse a CLI identifier, accepting exactly the registered spellings. - pub fn parse(s: &str) -> Result { + pub fn parse(s: &str) -> Result { Self::all() .iter() .copied() .find(|id| id.as_str() == s) - .ok_or_else(|| { - format!( - "Unknown language/template '{}'. Supported values are: {}.", - s, - Self::all() - .iter() - .map(|id| id.as_str()) - .collect::>() - .join(", ") - ) - }) + .ok_or_else(|| ValidationError::UnknownTemplate { id: s.to_string() }) } /// Human-readable menu label of this template, as offered by the wizard @@ -407,21 +397,132 @@ pub struct NewCli { pub defaults: bool, } +/// Failure of one of the scaffold option validators ([`validate_source_name`], +/// [`validate_command`], [`validate_upstream_version`], [`parse_maintainer`], +/// [`validate_homepage`], [`validate_depends`], [`validate_orig_path`], +/// [`check_file_collisions`], [`TemplateId::parse`]): what the wizard re-asks +/// about, carrying the pieces callers may want to match on. +#[derive(Debug, thiserror::Error)] +pub enum ValidationError { + /// The source package name is shorter than dpkg's 2-character minimum. + #[error("'{name}' is too short: package names need at least 2 characters")] + SourceNameTooShort { + /// The rejected name. + name: String, + }, + /// The source package name is outside dpkg's charset or shape. + #[error( + "'{}' is not a valid package name: names must be lowercase, start with a letter or digit and may only contain [a-z0-9+.-]", + name + )] + InvalidSourceName { + /// The rejected name. + name: String, + }, + /// No installed command was given. + #[error("the command name must not be empty")] + EmptyCommand, + /// The command name is not a lowercase identifier. + #[error( + "'{}' is not a valid command name: commands must be a lowercase identifier (letters, digits, + - . _), e.g. 'mytool'", + command + )] + InvalidCommand { + /// The rejected command name. + command: String, + }, + /// Upstream versions should start with a digit (dpkg recommendation, + /// enforced here). + #[error("upstream versions should start with a digit")] + UpstreamNotDigit, + /// `-` is the Debian revision separator. + #[error( + "upstream versions must not contain '-': the Debian revision is appended automatically" + )] + UpstreamContainsDash, + /// The upstream version composed with the Debian revision does not + /// parse as a Debian version. + #[error("'{version}' is not a valid Debian version: {source}")] + InvalidVersion { + /// The composed `upstream-revision` string. + version: String, + /// The version parse failure. + source: crate::debian::version::VersionParseError, + }, + /// The maintainer is not of the form `Name `. + #[error("'{s}' is not a valid maintainer: expected the form 'Name '")] + MaintainerShape { + /// The rejected maintainer string. + s: String, + }, + /// The maintainer name (before the angle brackets) is empty. + #[error("'{s}': the maintainer name must not be empty")] + MaintainerNameEmpty { + /// The rejected maintainer string. + s: String, + }, + /// The maintainer email does not carry exactly one `@`. + #[error("'{s}': the maintainer email must contain exactly one '@'")] + MaintainerEmailAt { + /// The rejected maintainer string. + s: String, + }, + /// The maintainer email carries whitespace. + #[error("'{s}': the maintainer email must not contain whitespace")] + MaintainerEmailWhitespace { + /// The rejected maintainer string. + s: String, + }, + /// The homepage is not an http(s) URL. + #[error("'{url}' is not a valid homepage: expected an http:// or https:// URL")] + InvalidHomepage { + /// The rejected URL. + url: String, + }, + /// The Depends list does not parse as package relations. + #[error("Invalid Depends list '{list}': {source}")] + InvalidDepends { + /// The rejected list. + list: String, + /// The relation parse failure. + source: crate::debian::deps::ParseError, + }, + /// The orig tarball is neither an http(s) URL nor an existing file. + #[error( + "'{path}' is not an http(s) URL and not an existing file: --orig-path must point at a tarball to repack" + )] + InvalidOrigPath { + /// The rejected path or URL. + path: String, + }, + /// The language/template id is not one of the registered spellings. + #[error("Unknown language/template '{}'. Supported values are: {}.", id, TemplateId::all().iter().map(|id| id.as_str()).collect::>().join(", "))] + UnknownTemplate { + /// The unknown id. + id: String, + }, + /// A template generated the same file twice. + #[error("internal error: file '{path}' was generated more than once")] + GeneratedTwice { + /// The duplicated path. + path: String, + }, +} + /// Validate a Debian source package name: `^[a-z0-9][a-z0-9+.\-]+$` with a /// minimum length of 2 (dpkg rules). -pub fn validate_source_name(name: &str) -> Result<(), String> { +pub fn validate_source_name(name: &str) -> Result<(), ValidationError> { static NAME_REGEX: std::sync::OnceLock = std::sync::OnceLock::new(); let regex = NAME_REGEX.get_or_init(|| Regex::new(r"^[a-z0-9][a-z0-9+.\-]+$").unwrap()); if name.len() < 2 { - return Err(format!( - "'{name}' is too short: package names need at least 2 characters" - )); + return Err(ValidationError::SourceNameTooShort { + name: name.to_string(), + }); } if !regex.is_match(name) { - return Err(format!( - "'{name}' is not a valid package name: names must be lowercase, \ - start with a letter or digit and may only contain [a-z0-9+.-]" - )); + return Err(ValidationError::InvalidSourceName { + name: name.to_string(), + }); } Ok(()) } @@ -473,17 +574,16 @@ pub fn sanitize_name(input: &str) -> Option { /// `meson.build` and the `[project.scripts]` table, so uppercase letters, /// whitespace, quotes and shell metacharacters are rejected here instead of /// generating broken install lines and build files. -pub fn validate_command(command: &str) -> Result<(), String> { +pub fn validate_command(command: &str) -> Result<(), ValidationError> { static COMMAND_REGEX: std::sync::OnceLock = std::sync::OnceLock::new(); let regex = COMMAND_REGEX.get_or_init(|| Regex::new(r"^[a-z0-9][a-z0-9+.\-_]*$").unwrap()); if command.is_empty() { - return Err("the command name must not be empty".to_string()); + return Err(ValidationError::EmptyCommand); } if !regex.is_match(command) { - return Err(format!( - "'{command}' is not a valid command name: commands must be a \ - lowercase identifier (letters, digits, + - . _), e.g. 'mytool'" - )); + return Err(ValidationError::InvalidCommand { + command: command.to_string(), + }); } Ok(()) } @@ -492,65 +592,60 @@ pub fn validate_command(command: &str) -> Result<(), String> { /// recommendation, enforced here) and survive [`DebianVersion::parse`] once /// composed with the Debian revision. It must not contain `-` (the revision /// separator). -pub fn validate_upstream_version(upstream: &str, revision: u32) -> Result<(), String> { +pub fn validate_upstream_version(upstream: &str, revision: u32) -> Result<(), ValidationError> { if !upstream.starts_with(|c: char| c.is_ascii_digit()) { - return Err("upstream versions should start with a digit".to_string()); + return Err(ValidationError::UpstreamNotDigit); } if upstream.contains('-') { - return Err( - "upstream versions must not contain '-': the Debian revision is \ - appended automatically" - .to_string(), - ); + return Err(ValidationError::UpstreamContainsDash); } let composed = format!("{}-{}", upstream, revision); DebianVersion::parse(&composed) .map(|_| ()) - .map_err(|e| format!("'{composed}' is not a valid Debian version: {e}")) + .map_err(|source| ValidationError::InvalidVersion { + version: composed.clone(), + source, + }) } /// Parse a `Name ` maintainer string: the name must be non-empty and /// the email must contain exactly one `@` with no whitespace inside the /// angle brackets. -pub fn parse_maintainer(s: &str) -> Result<(String, String), String> { +pub fn parse_maintainer(s: &str) -> Result<(String, String), ValidationError> { static MAINTAINER_REGEX: std::sync::OnceLock = std::sync::OnceLock::new(); let regex = MAINTAINER_REGEX.get_or_init(|| Regex::new(r"^(.+?)\s*<([^<>]*)>\s*$").unwrap()); - let caps = regex.captures(s.trim()).ok_or_else(|| { - format!("'{s}' is not a valid maintainer: expected the form 'Name '") - })?; + let caps = regex + .captures(s.trim()) + .ok_or_else(|| ValidationError::MaintainerShape { s: s.to_string() })?; let name = caps[1].trim().to_string(); let email = caps[2].trim().to_string(); if name.is_empty() { - return Err(format!("'{s}': the maintainer name must not be empty")); + return Err(ValidationError::MaintainerNameEmpty { s: s.to_string() }); } if !email.contains('@') || email.matches('@').count() > 1 { - return Err(format!( - "'{s}': the maintainer email must contain exactly one '@'" - )); + return Err(ValidationError::MaintainerEmailAt { s: s.to_string() }); } if email.chars().any(char::is_whitespace) { - return Err(format!( - "'{s}': the maintainer email must not contain whitespace" - )); + return Err(ValidationError::MaintainerEmailWhitespace { s: s.to_string() }); } Ok((name, email)) } /// Validate a homepage URL: it must carry an `http(s)://` prefix. -pub fn validate_homepage(url: &str) -> Result<(), String> { +pub fn validate_homepage(url: &str) -> Result<(), ValidationError> { if url.starts_with("http://") || url.starts_with("https://") { Ok(()) } else { - Err(format!( - "'{url}' is not a valid homepage: expected an http:// or https:// URL" - )) + Err(ValidationError::InvalidHomepage { + url: url.to_string(), + }) } } /// Validate a comma-separated runtime Depends list (the metapackage flavor) /// by parsing it with [`Deps::parse`] and re-rendering every clause /// canonically. -pub fn validate_depends(list: &str) -> Result, String> { +pub fn validate_depends(list: &str) -> Result, ValidationError> { let opts = ParseOpts { host_arch: crate::get_current_arch(), build_arch: crate::get_current_arch(), @@ -559,8 +654,10 @@ pub fn validate_depends(list: &str) -> Result, String> { union: false, build_dep: false, }; - let deps = - Deps::parse(list, &opts).map_err(|e| format!("Invalid Depends list '{list}': {e}"))?; + let deps = Deps::parse(list, &opts).map_err(|source| ValidationError::InvalidDepends { + list: list.to_string(), + source, + })?; Ok(deps .clauses() .map(|clause| { @@ -620,7 +717,7 @@ pub async fn resolve(cli: NewCli) -> Result { // (nothing to detect for a fresh skeleton), else --defaults picks the // empty template. let template = match &cli.lang { - Some(lang) => Some(TemplateId::parse(lang)?), + Some(lang) => Some(TemplateId::parse(lang).map_err(|e| e.to_string())?), None => match &source_dir { SourceDir::Skeleton => { if cli.defaults { @@ -706,7 +803,7 @@ pub async fn resolve(cli: NewCli) -> Result { } // Everything below has a default and is validated as it is resolved. - validate_source_name(&name)?; + validate_source_name(&name).map_err(|e| e.to_string())?; // Source format: explicit flags win (--native / --quilt, mutually // exclusive), otherwise the mode decides — a fresh, self-authored @@ -769,7 +866,7 @@ pub async fn resolve(cli: NewCli) -> Result { let homepage = match &cli.homepage { Some(h) => { - validate_homepage(h)?; + validate_homepage(h).map_err(|e| e.to_string())?; Some(h.clone()) } None => None, @@ -780,10 +877,10 @@ pub async fn resolve(cli: NewCli) -> Result { let command = cli.command.unwrap_or_else(|| name.clone()); // The default is the already-validated package name (a subset of the // command charset), so only an explicit --command/wizard answer can fail. - validate_command(&command)?; + validate_command(&command).map_err(|e| e.to_string())?; let maintainer = match &cli.maintainer { - Some(m) => parse_maintainer(m)?, + Some(m) => parse_maintainer(m).map_err(|e| e.to_string())?, None => crate::changelog::get_maintainer_info().map_err(|e| { format!( "Could not determine the maintainer ({e}). \ @@ -815,7 +912,7 @@ pub async fn resolve(cli: NewCli) -> Result { let depends = if joined.trim().is_empty() { Vec::new() } else { - validate_depends(&joined)? + validate_depends(&joined).map_err(|e| e.to_string())? }; Ok(NewOptions { @@ -874,7 +971,7 @@ fn resolve_orig_origin( let source = cli.orig_path.clone().ok_or_else(|| { "--orig-from path requires --orig-path ".to_string() })?; - validate_orig_path(&source)?; + validate_orig_path(&source).map_err(|e| e.to_string())?; OrigOrigin::Provided { source } } Some("git") => { @@ -935,27 +1032,26 @@ fn resolve_orig_origin( /// Validate a `--orig-path` value (and the wizard's tarball answer): either /// an http(s) URL (downloaded at scaffold time) or an existing local /// tarball. -pub fn validate_orig_path(source: &str) -> Result<(), String> { +pub fn validate_orig_path(source: &str) -> Result<(), ValidationError> { if source.starts_with("http://") || source.starts_with("https://") { return Ok(()); } if Path::new(source).is_file() { return Ok(()); } - Err(format!( - "'{source}' is not an http(s) URL and not an existing file: \ - --orig-path must point at a tarball to repack" - )) + Err(ValidationError::InvalidOrigPath { + path: source.to_string(), + }) } /// Set of names no template may produce twice (collision check). -pub(crate) fn check_file_collisions(paths: &[String]) -> Result<(), String> { +pub(crate) fn check_file_collisions(paths: &[String]) -> Result<(), ValidationError> { let mut seen: HashSet<&String> = HashSet::new(); for path in paths { if !seen.insert(path) { - return Err(format!( - "internal error: file '{path}' was generated more than once" - )); + return Err(ValidationError::GeneratedTwice { + path: path.to_string(), + }); } } Ok(()) @@ -1222,7 +1318,9 @@ mod tests { #[test] fn collision_detection() { assert!(check_file_collisions(&["a".into(), "b".into()]).is_ok()); - let err = check_file_collisions(&["a".into(), "b".into(), "a".into()]).unwrap_err(); + let err = check_file_collisions(&["a".into(), "b".into(), "a".into()]) + .unwrap_err() + .to_string(); assert!(err.contains('a')); } diff --git a/src/new/questions.rs b/src/new/questions.rs index 6dc14fa..57a7c80 100644 --- a/src/new/questions.rs +++ b/src/new/questions.rs @@ -96,12 +96,9 @@ async fn run_wizard( // basename of the current directory. if cli.name.is_none() { let default = default_package_name(&cwd, probe.as_ref()); - let answer = ask_text( - prompter, - "Package name", - &default, - options::validate_source_name, - )?; + let answer = ask_text(prompter, "Package name", &default, |name: &str| { + options::validate_source_name(name).map_err(|e| e.to_string()) + })?; cli.name = Some(answer); } @@ -265,7 +262,9 @@ async fn run_wizard( .or_else(|| origin.as_ref().and_then(GitOrigin::git_version)) .unwrap_or_else(|| "0.1.0".to_string()); let revision = cli.revision.unwrap_or(1); - let validate = move |version: &str| options::validate_upstream_version(version, revision); + let validate = move |version: &str| { + options::validate_upstream_version(version, revision).map_err(|e| e.to_string()) + }; let answer = ask_text(prompter, "Upstream version", &default, validate)?; cli.upstream_version = Some(answer.clone()); @@ -321,7 +320,8 @@ async fn run_wizard( .expect("answer comes from the choice list"); cli.orig_from = Some(chosen.to_string()); if chosen == "path" { - let validator = |path: &str| options::validate_orig_path(path); + let validator = + |path: &str| options::validate_orig_path(path).map_err(|e| e.to_string()); let path = prompter.text("Tarball path or URL", "", Some(&validator))?; cli.orig_path = Some(path); } @@ -354,7 +354,7 @@ async fn run_wizard( if url.is_empty() { Ok(()) } else { - options::validate_homepage(url) + options::validate_homepage(url).map_err(|e| e.to_string()) } }; let answer = ask_text(prompter, "Homepage (blank to skip)", &default, validate)?; @@ -400,12 +400,9 @@ async fn run_wizard( .as_ref() .and_then(|p| p.command.clone()) .unwrap_or_else(|| cli.name.clone().unwrap_or_default()); - let command = ask_text( - prompter, - "Command name", - &default, - options::validate_command, - )?; + let command = ask_text(prompter, "Command name", &default, |command: &str| { + options::validate_command(command).map_err(|e| e.to_string()) + })?; cli.command = Some(command); } @@ -430,7 +427,11 @@ async fn run_wizard( DEBEMAIL and no git user config): answer as 'Name '" ); } - let validate = |answer: &str| options::parse_maintainer(answer).map(|_| ()); + let validate = |answer: &str| { + options::parse_maintainer(answer) + .map(|_| ()) + .map_err(|e| e.to_string()) + }; cli.maintainer = Some(ask_text(prompter, "Maintainer", &default, validate)?); } @@ -480,7 +481,11 @@ async fn run_wizard( // 13. Metapackage Depends (empty template only). if template == TemplateId::EMPTY && cli.depends.is_empty() { - let validate = |answer: &str| options::validate_depends(answer).map(|_| ()); + let validate = |answer: &str| { + options::validate_depends(answer) + .map(|_| ()) + .map_err(|e| e.to_string()) + }; let answer = ask_text( prompter, "Depends (metapackage, comma-separated, blank for an empty base)", @@ -1496,7 +1501,9 @@ mod tests { assert!(options::validate_upstream_version("v1.0", 1).is_err()); assert!(options::validate_upstream_version("1.0.0", 1).is_ok()); - let validate = |version: &str| options::validate_upstream_version(version, 1); + let validate = |version: &str| { + options::validate_upstream_version(version, 1).map_err(|e| e.to_string()) + }; // Invalid probes are not offered as the default... assert_eq!(offered_default("1.0-2", &validate), ""); assert_eq!(offered_default("v1.0", &validate), ""); @@ -1532,7 +1539,7 @@ mod tests { // The upstream-version scenario end to end: Enter on an invalid // probed default (offered as none) fails instead of taking it // verbatim, while a valid default is taken. - let version = |v: &str| options::validate_upstream_version(v, 1); + let version = |v: &str| options::validate_upstream_version(v, 1).map_err(|e| e.to_string()); assert!(accept_answer("", "1.0-2", &version).is_err()); assert_eq!( accept_answer("", "1.0.0", &version),