From 3b60637e153642925fa96021a302bc5be504b2e5 Mon Sep 17 00:00:00 2001 From: Michael-F-Bryan Date: Fri, 12 Jun 2020 00:20:58 +0800 Subject: [PATCH 1/3] Refactored interpolate_env() to be more testable --- Cargo.lock | 1 + Cargo.toml | 1 + src/config.rs | 166 ++++++++++++++++++++++++++++++++++---------------- 3 files changed, 114 insertions(+), 54 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 503192e96..ef45102cc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -810,6 +810,7 @@ dependencies = [ "dunce", "env_logger 0.7.1", "http", + "lazy_static", "linkcheck", "log", "mdbook", diff --git a/Cargo.toml b/Cargo.toml index 90c038aef..26a67b4f2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,6 +32,7 @@ serde_derive = "1.0" serde_json = "1.0" structopt = "0.3" tokio = "0.2.20" +lazy_static = "1.4.0" [dev-dependencies] pretty_assertions = "0.6.1" diff --git a/src/config.rs b/src/config.rs index 67efdc890..321dbe3bd 100644 --- a/src/config.rs +++ b/src/config.rs @@ -2,6 +2,7 @@ use crate::hashed_regex::HashedRegex; use anyhow::Error; use http::header::{HeaderName, HeaderValue}; use log::Level; +use regex::{Captures, Regex, Replacer}; use reqwest::Client; use serde_derive::{Deserialize, Serialize}; use std::{ @@ -49,7 +50,7 @@ pub struct HttpHeader { impl HttpHeader { pub(crate) fn interpolate(&self) -> Result { - interpolate_env(&self.value) + interpolate_env(&self.value, |var| std::env::var(var).ok()) } } @@ -179,70 +180,91 @@ impl Into for HttpHeader { fn default_cache_timeout() -> u64 { Config::DEFAULT_CACHE_TIMEOUT.as_secs() } fn default_user_agent() -> String { Config::DEFAULT_USER_AGENT.to_string() } -fn interpolate_env(value: &str) -> Result { - use std::{iter::Peekable, str::CharIndices}; +lazy_static::lazy_static! { + static ref INTERPOLATED_VARIABLE: Regex = Regex::new(r"(?x) + (?P\\)? + \$ + (?P[\w_][\w_\d]*) + ").unwrap(); +} - fn is_ident(ch: char) -> bool { ch.is_ascii_alphanumeric() || ch == '_' } +fn interpolate_env(value: &str, get_var: F) -> Result +where + F: FnMut(&str) -> Option, +{ + let mut failed_replacements: Vec = Vec::new(); + + let interpolated = INTERPOLATED_VARIABLE + .replace_all(value, replacer(&mut failed_replacements, get_var)); + + if failed_replacements.is_empty() { + interpolated.parse().map_err(Error::from) + } else { + Err(Error::from(InterpolationError { + variable_names: failed_replacements, + original_string: value.to_string(), + })) + } +} - fn ident_end(start: usize, iter: &mut Peekable) -> usize { - let mut end = start; - while let Some(&(i, ch)) = iter.peek() { - if !is_ident(ch) { - return i; +/// Gets a `Replacer` which will try to replace a variable with the result +/// from the `get_var()` function, recording any errors that happen. +fn replacer<'a, V>( + failed_replacements: &'a mut Vec, + mut get_var: V, +) -> impl Replacer + 'a +where + V: FnMut(&str) -> Option + 'a, +{ + move |caps: &Captures<'_>| { + if caps.name("escape").is_none() { + let variable = &caps["variable"]; + + match get_var(variable) { + Some(value) => return value, + None => { + failed_replacements.push(variable.to_string()); + }, } - end = i + ch.len_utf8(); - iter.next(); } - end + // the dollar sign was escaped (e.g. "\$foo") or we couldn't get + // the environment variable + caps[0].to_string() } +} - let mut res = String::with_capacity(value.len()); - let mut backslash = false; - let mut iter = value.char_indices().peekable(); - - while let Some((i, ch)) = iter.next() { - if backslash { - match ch { - '$' | '\\' => res.push(ch), - _ => { - res.push('\\'); - res.push(ch); - }, - } +#[derive(Debug, Clone)] +struct InterpolationError { + pub variable_names: Vec, + pub original_string: String, +} - backslash = false; - } else { - match ch { - '\\' => backslash = true, - '$' => { - iter.next(); - let start = i + 1; - let end = ident_end(start, &mut iter); - let name = &value[start..end]; - - match std::env::var(name) { - Ok(env) => res.push_str(&env), - Err(e) => { - return Err(Error::msg(format!( - "Failed to retrieve `{}` env var: {}", - name, e - ))) - }, - } - }, +impl std::error::Error for InterpolationError {} - _ => res.push(ch), - } +impl Display for InterpolationError { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + if self.variable_names.len() == 1 { + write!( + f, + "Unable to interpolate `${}` into \"{}\"", + &self.variable_names[0], self.original_string + ) + } else { + let formatted_names: Vec<_> = self + .variable_names + .iter() + .map(|v| format!("`${}`", v)) + .collect(); + + write!( + f, + "Unable to interpolate `${}` into \"{}\"", + formatted_names.join(", "), + self.original_string + ) } } - - // trailing backslash - if backslash { - res.push('\\'); - } - - Ok(res.parse()?) } /// How should warnings be treated? @@ -338,4 +360,40 @@ https = ["accept: html/text", "authorization: Basic $TOKEN"] assert_eq!(got, should_be); } + + #[test] + fn interplate_a_single_variable() { + let text = "Hello, $name"; + + let got = interpolate_env(text, |name| { + if name == "name" { + Some(String::from("World!")) + } else { + None + } + }) + .unwrap(); + + assert_eq!(got, "Hello, World!"); + } + + #[test] + fn you_can_skip_interpolation_by_escaping_the_dollar_sign() { + let text = r"Hello, \$name"; + + let got = interpolate_env(text, |_| unreachable!()).unwrap(); + + assert_eq!(got, text); + } + + #[test] + fn not_having_the_requested_variable_is_an_error() { + let text = r"Hello, $name"; + let never_works = |_name: &str| None; + + let got = interpolate_env(text, never_works).unwrap_err(); + + let inner = got.downcast::().unwrap(); + assert_eq!(inner.variable_names, vec![String::from("name")]); + } } From 0505e144c95c66f5ea6abd853f00302a7d16d374 Mon Sep 17 00:00:00 2001 From: Michael-F-Bryan Date: Fri, 12 Jun 2020 01:01:09 +0800 Subject: [PATCH 2/3] Migrated all diagnostics over to an `ErrorHandling` type --- src/config.rs | 50 ++------ src/error_handling.rs | 283 ++++++++++++++++++++++++++++++++++++++++++ src/lib.rs | 6 +- src/validate.rs | 212 +++---------------------------- tests/smoke_tests.rs | 8 +- 5 files changed, 319 insertions(+), 240 deletions(-) create mode 100644 src/error_handling.rs diff --git a/src/config.rs b/src/config.rs index 321dbe3bd..24181f2b2 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,4 +1,4 @@ -use crate::hashed_regex::HashedRegex; +use crate::{error_handling::ErrorHandling, hashed_regex::HashedRegex}; use anyhow::Error; use http::header::{HeaderName, HeaderValue}; use log::Level; @@ -32,13 +32,13 @@ pub struct Config { /// The number of seconds a cached result is valid for. #[serde(default = "default_cache_timeout")] pub cache_timeout: u64, - /// The policy to use when warnings are encountered. - #[serde(default)] - pub warning_policy: WarningPolicy, /// The map of regexes representing sets of web sites and /// the list of HTTP headers that must be sent to matching sites. #[serde(default)] pub http_headers: HashMap>, + /// How should non-valid links be handled? + #[serde(default)] + pub error_handling: ErrorHandling, } #[derive(Serialize, Deserialize, Debug, PartialEq, Clone)] @@ -83,10 +83,9 @@ impl Config { pub(crate) fn interpolate_headers( &self, - warning_policy: WarningPolicy, + error_handling: &ErrorHandling, ) -> Vec<(HashedRegex, Vec<(HeaderName, HeaderValue)>)> { let mut all_headers = Vec::new(); - let log_level = warning_policy.to_log_level(); for (pattern, headers) in &self.http_headers { let mut interpolated = Vec::new(); @@ -103,12 +102,8 @@ impl Config { // // If it was important, the user would notice a "broken" // link and read back through the logs. - log::log!( - log_level, - "Unable to interpolate \"{}\" because {}", - header, - e - ); + error_handling + .on_header_interpolation_error(header, &e); }, } } @@ -128,8 +123,8 @@ impl Default for Config { exclude: Vec::new(), user_agent: default_user_agent(), http_headers: HashMap::new(), - warning_policy: WarningPolicy::Warn, cache_timeout: Config::DEFAULT_CACHE_TIMEOUT.as_secs(), + error_handling: ErrorHandling::default(), } } } @@ -267,32 +262,6 @@ impl Display for InterpolationError { } } -/// How should warnings be treated? -#[derive(Debug, Copy, Clone, PartialEq, Serialize, Deserialize)] -#[serde(rename_all = "kebab-case")] -pub enum WarningPolicy { - /// Silently ignore them. - Ignore, - /// Warn the user, but don't fail the linkcheck. - Warn, - /// Treat warnings as errors. - Error, -} - -impl WarningPolicy { - pub(crate) fn to_log_level(self) -> Level { - match self { - WarningPolicy::Error => Level::Error, - WarningPolicy::Warn => Level::Warn, - WarningPolicy::Ignore => Level::Debug, - } - } -} - -impl Default for WarningPolicy { - fn default() -> WarningPolicy { WarningPolicy::Warn } -} - #[cfg(test)] mod tests { use super::*; @@ -304,7 +273,6 @@ traverse-parent-directories = true exclude = ["google\\.com"] user-agent = "Internet Explorer" cache-timeout = 3600 -warning-policy = "error" [http-headers] https = ["accept: html/text", "authorization: Basic $TOKEN"] @@ -316,7 +284,6 @@ https = ["accept: html/text", "authorization: Basic $TOKEN"] let should_be = Config { follow_web_links: true, - warning_policy: WarningPolicy::Error, traverse_parent_directories: true, exclude: vec![HashedRegex::new(r"google\.com").unwrap()], user_agent: String::from("Internet Explorer"), @@ -328,6 +295,7 @@ https = ["accept: html/text", "authorization: Basic $TOKEN"] ], )]), cache_timeout: 3600, + error_handling: ErrorHandling::default(), }; let got: Config = toml::from_str(CONFIG).unwrap(); diff --git a/src/error_handling.rs b/src/error_handling.rs new file mode 100644 index 000000000..512192e3d --- /dev/null +++ b/src/error_handling.rs @@ -0,0 +1,283 @@ +use crate::{config::HttpHeader, IncompleteLink}; +use anyhow::Error; +use codespan::{FileId, Files, Span}; +use codespan_reporting::diagnostic::{Diagnostic, Label, Severity}; +use linkcheck::{ + validation::{InvalidLink, Reason}, + Link, +}; +use serde::{ + de::{Deserialize, Deserializer}, + ser::{Serialize, SerializeMap, Serializer}, +}; +use std::{ + collections::BTreeMap, + path::{Component, Path, PathBuf}, +}; + +const ABSOLUTE_LINK_WARNING_REASONING: &'static str = r#"When viewing a document directly from the file system and click on an +absolute link (e.g. `/index.md`), the browser will try to navigate to +`/index.md` on the current file system (i.e. the `index.md` file inside +`/` or `C:\`) instead of the `index.md` file at book's base directory as +intended. + +This warning helps avoid the situation where everything will seem to work +fine when viewed using a web server (e.g. GitHub Pages or `mdbook serve`), +but users viewing the book from the file system may encounter broken links. + +To ignore this warning, you can edit `book.toml` and set the warning policy to +"ignore". + + [output.linkcheck] + warning-policy = "ignore" + +For more details, see https://github.com/Michael-F-Bryan/mdbook-linkcheck/issues/33 +"#; + +/// The policy used when emitting errors or warnings. +#[derive(Debug, Clone, PartialEq)] +pub struct ErrorHandling { + rules: Vec, +} + +impl ErrorHandling { + pub(crate) fn on_incomplete_link( + &self, + link: &IncompleteLink, + span: Span, + ) -> Option> { + let IncompleteLink { ref text, file } = link; + let severity = Severity::Error; + + let msg = format!("Did you forget to define a URL for `{0}`?", text); + let label = Label::primary(*file, span).with_message(msg); + let note = format!( + "hint: declare the link's URL. For example: `[{}]: http://example.com/`", + text + ); + + let diag = Diagnostic::new(severity) + .with_message("Potential incomplete link") + .with_labels(vec![label]) + .with_notes(vec![note]); + + Some(diag) + } + + pub(crate) fn on_invalid_link( + &self, + broken: &InvalidLink, + ) -> Option> { + let msg = most_specific_error_message(broken); + let link = &broken.link; + + let diag = + Diagnostic::error() + .with_message(msg.clone()) + .with_labels(vec![ + Label::primary(link.file, link.span).with_message(msg) + ]); + + Some(diag) + } + + pub(crate) fn on_absolute_link( + &self, + link: &Link, + insert_explanatory_text: bool, + files: &Files, + ) -> Option> { + let severity = Severity::Error; + + let mut notes = Vec::new(); + + if insert_explanatory_text { + notes.push(String::from(ABSOLUTE_LINK_WARNING_REASONING)); + } + + if let Some(suggested_change) = + relative_path_to_file(files.name(link.file), &link.href) + { + notes.push(format!( + "Suggestion: change the link to \"{}\"", + suggested_change + )); + } + + let diag = Diagnostic::new(severity) + .with_message("Absolute link should be made relative") + .with_notes(notes) + .with_labels(vec![Label::primary(link.file, link.span) + .with_message("Absolute link should be made relative")]); + + Some(diag) + } + + pub(crate) fn on_header_interpolation_error( + &self, + header: &HttpHeader, + error: &Error, + ) { + let log_level = log::Level::Warn; + + log::log!( + log_level, + "Unable to interpolate \"{}\" because {}", + header, + error, + ); + } +} + +// Path diffing, copied from https://crates.io/crates/pathdiff with some tweaks +fn relative_path_to_file(start: S, destination: D) -> Option +where + S: AsRef, + D: AsRef, +{ + let destination = destination.as_ref(); + let start = start.as_ref(); + log::debug!( + "Trying to find the relative path from \"{}\" to \"{}\"", + start.display(), + destination.display() + ); + + let start = start.parent()?; + let destination_name = destination.file_name()?; + let destination = destination.parent()?; + + let mut ita = destination.components().skip(1); + let mut itb = start.components(); + + let mut comps: Vec = vec![]; + + loop { + match (ita.next(), itb.next()) { + (None, None) => break, + (Some(a), None) => { + comps.push(a); + comps.extend(ita.by_ref()); + break; + }, + (None, _) => comps.push(Component::ParentDir), + (Some(a), Some(b)) if comps.is_empty() && a == b => (), + (Some(a), Some(b)) if b == Component::CurDir => comps.push(a), + (Some(_), Some(b)) if b == Component::ParentDir => return None, + (Some(a), Some(_)) => { + comps.push(Component::ParentDir); + for _ in itb { + comps.push(Component::ParentDir); + } + comps.push(a); + comps.extend(ita.by_ref()); + break; + }, + } + } + + let path: PathBuf = comps + .iter() + .map(|c| c.as_os_str()) + .chain(std::iter::once(destination_name)) + .collect(); + + // Note: URLs always use forward slashes + Some(path.display().to_string().replace('\\', "/")) +} + +fn most_specific_error_message(link: &InvalidLink) -> String { + if link.reason.file_not_found() { + return format!("File not found: {}", link.link.href); + } + + match link.reason { + Reason::Io(ref io) => io.to_string(), + Reason::Web(ref web) if web.is_status() => { + let status = web.status().expect( + "Response::error_for_status() always contains a status code", + ); + let url = web + .url() + .expect("Response::error_for_status() always contains a URL"); + + match status.canonical_reason() { + Some(reason) => format!( + "Server returned {} {} for {}", + status.as_u16(), + reason, + url + ), + None => { + format!("Server returned {} for {}", status.as_u16(), url) + }, + } + }, + Reason::Web(ref web) => web.to_string(), + // fall back to the Reason's Display impl + _ => link.reason.to_string(), + } +} + +impl Serialize for ErrorHandling { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + if self.rules.is_empty() { + return serializer.serialize_none(); + } + + let mut ser = serializer.serialize_map(Some(self.rules.len()))?; + + for rule in &self.rules { + rule.serialize_to(&mut ser)?; + } + + ser.end() + } +} + +impl<'de> Deserialize<'de> for ErrorHandling { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + unimplemented!() + } +} + +impl Default for ErrorHandling { + fn default() -> Self { ErrorHandling { rules: Vec::new() } } +} + +#[derive(Debug, Copy, Clone, PartialEq)] +enum Rule {} + +impl Rule { + fn serialize_to(&self, ser: &mut S) -> Result<(), S::Error> + where + S: SerializeMap, + { + unimplemented!() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn check_some_simple_relative_paths() { + let inputs = vec![ + ("index.md", "/other.md", "other.md"), + ("index.md", "/nested/other.md", "nested/other.md"), + ("nested/index.md", "/other.md", "../other.md"), + ]; + + for (start, destination, should_be) in inputs { + let got = relative_path_to_file(start, destination).unwrap(); + assert_eq!(got, should_be); + } + } +} diff --git a/src/lib.rs b/src/lib.rs index 4c294b20e..bde7883fb 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -25,13 +25,15 @@ pub const COMPATIBLE_MDBOOK_VERSIONS: &str = "^0.3.0"; mod config; mod context; +mod error_handling; mod hashed_regex; mod links; mod validate; pub use crate::{ - config::{Config, WarningPolicy}, + config::Config, context::Context, + error_handling::ErrorHandling, hashed_regex::HashedRegex, links::{extract as extract_links, IncompleteLink}, validate::{validate, NotInSummary, ValidationOutcome}, @@ -71,7 +73,7 @@ pub fn run( } let (files, outcome) = check_links(&ctx, &mut cache, &cfg)?; - let diags = outcome.generate_diagnostics(&files, cfg.warning_policy); + let diags = outcome.generate_diagnostics(&files, &cfg.error_handling); report_errors(&files, &diags, colour)?; save_cache(cache_file, &cache); diff --git a/src/validate.rs b/src/validate.rs index a6f12e48c..7dcc11764 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -1,4 +1,4 @@ -use crate::{Config, Context, IncompleteLink, WarningPolicy}; +use crate::{Config, Context, ErrorHandling, IncompleteLink}; use anyhow::Error; use codespan::{FileId, Files, Span}; use codespan_reporting::diagnostic::{Diagnostic, Label, Severity}; @@ -43,7 +43,7 @@ fn lc_validate( .set_default_file("README.md") .set_custom_validation(ensure_included_in_book(src_dir, file_names)); - let interpolated_headers = cfg.interpolate_headers(cfg.warning_policy); + let interpolated_headers = cfg.interpolate_headers(&cfg.error_handling); let ctx = Context { client: cfg.client(), @@ -213,62 +213,42 @@ impl ValidationOutcome { pub fn generate_diagnostics( &self, files: &Files, - warning_policy: WarningPolicy, + error_handling: &ErrorHandling, ) -> Vec> { let mut diags = Vec::new(); - self.add_invalid_link_diagnostics(&mut diags); - self.add_incomplete_link_diagnostics(warning_policy, &mut diags, files); - self.warn_on_absolute_links(warning_policy, &mut diags, files); + self.add_invalid_link_diagnostics(error_handling, &mut diags); + self.add_incomplete_link_diagnostics(error_handling, &mut diags, files); + self.warn_on_absolute_links(error_handling, &mut diags, files); diags } fn add_incomplete_link_diagnostics( &self, - warning_policy: WarningPolicy, + error_handling: &ErrorHandling, diags: &mut Vec>, files: &Files, ) { - let severity = match warning_policy { - WarningPolicy::Error => Severity::Error, - WarningPolicy::Warn => Severity::Warning, - WarningPolicy::Ignore => return, - }; - for incomplete in &self.incomplete_links { - let IncompleteLink { ref text, file } = incomplete; - let span = resolve_incomplete_link_span(incomplete, files); - let msg = - format!("Did you forget to define a URL for `{0}`?", text); - let label = Label::primary(*file, span).with_message(msg); - let note = format!( - "hint: declare the link's URL. For example: `[{}]: http://example.com/`", - text - ); - - let diag = Diagnostic::new(severity) - .with_message("Potential incomplete link") - .with_labels(vec![label]) - .with_notes(vec![note]); - diags.push(diag) + if let Some(diag) = + error_handling.on_incomplete_link(&incomplete, span) + { + diags.push(diag); + } } } fn add_invalid_link_diagnostics( &self, + error_handling: &ErrorHandling, diags: &mut Vec>, ) { for broken_link in &self.invalid_links { - let link = &broken_link.link; - let msg = most_specific_error_message(&broken_link); - let diag = Diagnostic::error() - .with_message(msg.clone()) - .with_labels(vec![ - Label::primary(link.file, link.span).with_message(msg) - ]); - diags.push(diag); + if let Some(diag) = error_handling.on_invalid_link(broken_link) { + diags.push(diag); + } } } @@ -277,34 +257,10 @@ impl ValidationOutcome { /// being read directly from the filesystem. fn warn_on_absolute_links( &self, - warning_policy: WarningPolicy, + error_handling: &ErrorHandling, diags: &mut Vec>, files: &Files, ) { - const WARNING_MESSAGE: &'static str = r#"When viewing a document directly from the file system and click on an -absolute link (e.g. `/index.md`), the browser will try to navigate to -`/index.md` on the current file system (i.e. the `index.md` file inside -`/` or `C:\`) instead of the `index.md` file at book's base directory as -intended. - -This warning helps avoid the situation where everything will seem to work -fine when viewed using a web server (e.g. GitHub Pages or `mdbook serve`), -but users viewing the book from the file system may encounter broken links. - -To ignore this warning, you can edit `book.toml` and set the warning policy to -"ignore". - - [output.linkcheck] - warning-policy = "ignore" - -For more details, see https://github.com/Michael-F-Bryan/mdbook-linkcheck/issues/33 -"#; - let severity = match warning_policy { - WarningPolicy::Error => Severity::Error, - WarningPolicy::Warn => Severity::Warning, - WarningPolicy::Ignore => return, - }; - let absolute_links = self .valid_links .iter() @@ -313,121 +269,10 @@ For more details, see https://github.com/Michael-F-Bryan/mdbook-linkcheck/issues let mut reasoning_emitted = false; for link in absolute_links { - let mut notes = Vec::new(); - - if !reasoning_emitted { - notes.push(String::from(WARNING_MESSAGE)); - reasoning_emitted = true; - } - - if let Some(suggested_change) = - relative_path_to_file(files.name(link.file), &link.href) - { - notes.push(format!( - "Suggestion: change the link to \"{}\"", - suggested_change - )); - } - - let diag = Diagnostic::new(severity) - .with_message("Absolute link should be made relative") - .with_notes(notes) - .with_labels(vec![Label::primary(link.file, link.span) - .with_message("Absolute link should be made relative")]); - - diags.push(diag); - } - } -} - -// Path diffing, copied from https://crates.io/crates/pathdiff with some tweaks -fn relative_path_to_file(start: S, destination: D) -> Option -where - S: AsRef, - D: AsRef, -{ - let destination = destination.as_ref(); - let start = start.as_ref(); - log::debug!( - "Trying to find the relative path from \"{}\" to \"{}\"", - start.display(), - destination.display() - ); - - let start = start.parent()?; - let destination_name = destination.file_name()?; - let destination = destination.parent()?; - - let mut ita = destination.components().skip(1); - let mut itb = start.components(); - - let mut comps: Vec = vec![]; - - loop { - match (ita.next(), itb.next()) { - (None, None) => break, - (Some(a), None) => { - comps.push(a); - comps.extend(ita.by_ref()); - break; - }, - (None, _) => comps.push(Component::ParentDir), - (Some(a), Some(b)) if comps.is_empty() && a == b => (), - (Some(a), Some(b)) if b == Component::CurDir => comps.push(a), - (Some(_), Some(b)) if b == Component::ParentDir => return None, - (Some(a), Some(_)) => { - comps.push(Component::ParentDir); - for _ in itb { - comps.push(Component::ParentDir); - } - comps.push(a); - comps.extend(ita.by_ref()); - break; - }, + error_handling.on_absolute_link(link, !reasoning_emitted, files); + reasoning_emitted = true; } } - - let path: PathBuf = comps - .iter() - .map(|c| c.as_os_str()) - .chain(std::iter::once(destination_name)) - .collect(); - - // Note: URLs always use forward slashes - Some(path.display().to_string().replace('\\', "/")) -} - -fn most_specific_error_message(link: &InvalidLink) -> String { - if link.reason.file_not_found() { - return format!("File not found: {}", link.link.href); - } - - match link.reason { - Reason::Io(ref io) => io.to_string(), - Reason::Web(ref web) if web.is_status() => { - let status = web.status().expect( - "Response::error_for_status() always contains a status code", - ); - let url = web - .url() - .expect("Response::error_for_status() always contains a URL"); - - match status.canonical_reason() { - Some(reason) => format!( - "Server returned {} {} for {}", - status.as_u16(), - reason, - url - ), - None => { - format!("Server returned {} for {}", status.as_u16(), url) - }, - } - }, - Reason::Web(ref web) => web.to_string(), - // fall back to the Reason's Display impl - _ => link.reason.to_string(), - } } /// HACK: this is a workaround for @@ -446,22 +291,3 @@ fn resolve_incomplete_link_span( None => files.source_span(incomplete.file), } } - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn check_some_simple_relative_paths() { - let inputs = vec![ - ("index.md", "/other.md", "other.md"), - ("index.md", "/nested/other.md", "nested/other.md"), - ("nested/index.md", "/other.md", "../other.md"), - ]; - - for (start, destination, should_be) in inputs { - let got = relative_path_to_file(start, destination).unwrap(); - assert_eq!(got, should_be); - } - } -} diff --git a/tests/smoke_tests.rs b/tests/smoke_tests.rs index da7541a7c..01bec99f5 100644 --- a/tests/smoke_tests.rs +++ b/tests/smoke_tests.rs @@ -5,7 +5,7 @@ use anyhow::Error; use codespan::{FileId, Files}; use linkcheck::validation::{Cache, Reason}; use mdbook::{renderer::RenderContext, MDBook}; -use mdbook_linkcheck::{Config, HashedRegex, ValidationOutcome, WarningPolicy}; +use mdbook_linkcheck::{Config, ErrorHandling, HashedRegex, ValidationOutcome}; use std::{ collections::HashMap, convert::TryInto, @@ -93,11 +93,11 @@ fn detect_when_a_linked_file_isnt_in_summary_md() { #[test] fn emit_valid_suggestions_on_absolute_links() { let root = test_dir().join("absolute-links"); + let error_handling = ErrorHandling::default(); TestRun::new(root) - .after_validation(|files, outcome, _| { - let diags = - outcome.generate_diagnostics(files, WarningPolicy::Error); + .after_validation(move |files, outcome, _| { + let diags = outcome.generate_diagnostics(files, &error_handling); let suggestions = vec![ "\"chapter_1.md\"", From cc093cfc581a06bd9faaa013ba50de0eedacef61 Mon Sep 17 00:00:00 2001 From: Michael-F-Bryan Date: Sat, 13 Jun 2020 17:23:43 +0800 Subject: [PATCH 3/3] Forgot to actually record the diagnostic --- src/validate.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/validate.rs b/src/validate.rs index 7dcc11764..d35901294 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -269,8 +269,12 @@ impl ValidationOutcome { let mut reasoning_emitted = false; for link in absolute_links { - error_handling.on_absolute_link(link, !reasoning_emitted, files); - reasoning_emitted = true; + if let Some(diag) = + error_handling.on_absolute_link(link, !reasoning_emitted, files) + { + diags.push(diag); + reasoning_emitted = true; + } } } }