Skip to content

Commit 2818a76

Browse files
simoncozenssimoncozens
committed
Be (slightly) more grown-up about error handling
1 parent 71cea65 commit 2818a76

File tree

16 files changed

+166
-108
lines changed

16 files changed

+166
-108
lines changed

‎fontspector-checkapi/src/check.rs

+34-27
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
1-
use crate::{font::FontCollection, Registry, Status, StatusList, Testable};
1+
use crate::{
2+
font::FontCollection, prelude::FixFnResult, status::CheckFnResult, Registry, Status, Testable,
3+
};
24

35
pub type CheckId = String;
46

@@ -8,10 +10,10 @@ pub struct Check<'a> {
810
pub title: &'a str,
911
pub rationale: Option<&'a str>,
1012
pub proposal: Option<&'a str>,
11-
pub check_one: Option<&'a dyn Fn(&Testable) -> StatusList>,
12-
pub check_all: Option<&'a dyn Fn(&FontCollection) -> StatusList>,
13-
pub hotfix: Option<&'a dyn Fn(&Testable) -> bool>,
14-
pub fix_source: Option<&'a dyn Fn(&Testable) -> bool>,
13+
pub check_one: Option<&'a dyn Fn(&Testable) -> CheckFnResult>,
14+
pub check_all: Option<&'a dyn Fn(&FontCollection) -> CheckFnResult>,
15+
pub hotfix: Option<&'a dyn Fn(&Testable) -> FixFnResult>,
16+
pub fix_source: Option<&'a dyn Fn(&Testable) -> FixFnResult>,
1517
pub applies_to: &'a str,
1618
}
1719

@@ -34,42 +36,47 @@ impl<'a> Check<'a> {
3436
.map_or(false, |ft| ft.applies(f))
3537
}
3638

39+
fn status_to_result(&'a self, status: Status, file: Option<&'a Testable>) -> CheckResult {
40+
CheckResult {
41+
status,
42+
check_id: self.id.to_string(),
43+
check_name: self.title.to_string(),
44+
check_rationale: self.rationale.map(|x| x.to_string()),
45+
filename: file.map(|x| x.filename.clone()),
46+
}
47+
}
48+
3749
pub fn run_one(&'a self, f: &'a Testable) -> Vec<CheckResult> {
3850
if let Some(check_one) = self.check_one {
39-
return check_one(f)
40-
.map(|r| CheckResult {
41-
status: r,
42-
check_id: self.id.to_string(),
43-
check_name: self.title.to_string(),
44-
check_rationale: self.rationale.map(|x| x.to_string()),
45-
filename: Some(f.filename.clone()),
46-
})
47-
.collect();
51+
match check_one(f) {
52+
Ok(results) => results.map(|r| self.status_to_result(r, Some(f))).collect(),
53+
Err(e) => {
54+
vec![self.status_to_result(Status::error(&format!("Error: {}", e)), Some(f))]
55+
}
56+
}
57+
} else {
58+
vec![]
4859
}
49-
vec![]
5060
}
5161

5262
pub fn run_all(&'a self, f: &'a FontCollection) -> Vec<CheckResult> {
5363
if let Some(check_all) = self.check_all {
54-
check_all(f)
55-
.map(|r| CheckResult {
56-
status: r,
57-
check_id: self.id.to_string(),
58-
check_name: self.title.to_string(),
59-
check_rationale: self.rationale.map(|x| x.to_string()),
60-
filename: None,
61-
})
62-
.collect()
64+
match check_all(f) {
65+
Ok(results) => results.map(|r| self.status_to_result(r, None)).collect(),
66+
Err(e) => {
67+
vec![self.status_to_result(Status::error(&format!("Error: {}", e)), None)]
68+
}
69+
}
6370
} else {
6471
vec![]
6572
}
6673
}
6774
}
6875

69-
pub fn return_result(problems: Vec<Status>) -> Box<dyn Iterator<Item = Status>> {
76+
pub fn return_result(problems: Vec<Status>) -> CheckFnResult {
7077
if problems.is_empty() {
71-
Status::just_one_pass()
78+
Ok(Status::just_one_pass())
7279
} else {
73-
Box::new(problems.into_iter())
80+
Ok(Box::new(problems.into_iter()))
7481
}
7582
}

‎fontspector-checkapi/src/filetype.rs

+5-1
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,11 @@ impl FileType<'_> {
2020
}
2121

2222
pub fn applies(&self, file: &Testable) -> bool {
23-
glob_match(self.pattern, &file.basename())
23+
if let Some(basename) = file.basename() {
24+
glob_match(self.pattern, &basename)
25+
} else {
26+
false
27+
}
2428
}
2529
}
2630

‎fontspector-checkapi/src/font.rs

+25-9
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use skrifa::{
66
string::{LocalizedStrings, StringId},
77
MetadataProvider, Tag,
88
};
9-
use std::{collections::HashSet, error::Error, path::Path};
9+
use std::{collections::HashSet, error::Error, io::ErrorKind, path::Path};
1010

1111
pub struct TestFont {
1212
filename: String,
@@ -19,13 +19,16 @@ pub const TTF: FileType = FileType { pattern: "*.ttf" };
1919

2020
impl<'a> FileTypeConvert<TestFont> for FileType<'a> {
2121
fn from_testable(&self, t: &Testable) -> Option<TestFont> {
22-
self.applies(t).then(|| TestFont::new(&t.filename))
22+
self.applies(t)
23+
.then(|| TestFont::new(&t.filename))
24+
.transpose()
25+
.unwrap_or(None)
2326
}
2427
}
2528

2629
impl TestFont {
27-
pub fn new(filename: &str) -> TestFont {
28-
let font_data = std::fs::read(filename).expect("Couldn't open file");
30+
pub fn new(filename: &str) -> std::io::Result<TestFont> {
31+
let font_data = std::fs::read(filename)?;
2932
let mut fnt = TestFont {
3033
filename: filename.to_string(),
3134
font_data,
@@ -40,19 +43,32 @@ impl TestFont {
4043
.collect();
4144
fnt._sibling_filenames = {
4245
// All other TTF files in same directory
43-
let directory = Path::new(&fnt.filename).parent().expect("Can't get parent");
44-
let paths = std::fs::read_dir(directory).expect("Can't read directory");
46+
let directory = Path::new(&fnt.filename)
47+
.parent()
48+
.ok_or(std::io::Error::new(
49+
ErrorKind::NotFound,
50+
"parent directory not found",
51+
))?;
52+
53+
let paths = std::fs::read_dir(directory)?;
4554
paths
4655
.flatten()
4756
.filter(|x| x.path().extension().map_or(false, |ext| ext == "ttf"))
4857
.filter(|x| x.path().to_string_lossy() != fnt.filename)
4958
.map(|x| x.path().to_string_lossy().to_string())
5059
.collect()
5160
};
52-
fnt
61+
if FontRef::new(&fnt.font_data).is_err() {
62+
return Err(std::io::Error::new(
63+
ErrorKind::InvalidData,
64+
"Can't parse font",
65+
));
66+
}
67+
Ok(fnt)
5368
}
5469
pub fn font(&self) -> FontRef {
55-
FontRef::new(&self.font_data).expect("Can't parse font")
70+
#[allow(clippy::expect_used)] // We just tested for it in the initializer
71+
FontRef::new(&self.font_data).expect("Can't happen")
5672
}
5773

5874
pub fn style(&self) -> Option<&str> {
@@ -79,7 +95,7 @@ impl TestFont {
7995
pub fn siblings(&self) -> Vec<TestFont> {
8096
self._sibling_filenames
8197
.iter()
82-
.map(|x| TestFont::new(x))
98+
.flat_map(|x| TestFont::new(x))
8399
.collect()
84100
}
85101

‎fontspector-checkapi/src/lib.rs

+5-2
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
#![deny(clippy::unwrap_used, clippy::expect_used)]
12
mod check;
23
mod constants;
34
mod filetype;
@@ -11,12 +12,14 @@ pub use filetype::{FileType, FileTypeConvert};
1112
pub use font::{FontCollection, TestFont, TTF};
1213
pub use profile::{Override, Profile};
1314
pub use registry::Registry;
14-
pub use status::{Status, StatusCode, StatusList};
15+
pub use status::{CheckFnResult, Status, StatusCode, StatusList};
1516
pub use testable::Testable;
1617

1718
pub mod prelude {
19+
pub type FixFnResult = Result<bool, String>;
1820
pub use crate::{
19-
return_result, Check, FileType, Profile, Registry, Status, StatusList, Testable, TTF,
21+
return_result, Check, CheckFnResult, FileType, Profile, Registry, Status, StatusList,
22+
Testable, TTF,
2023
};
2124
}
2225
pub trait Plugin {

‎fontspector-checkapi/src/profile.rs

+5-5
Original file line numberDiff line numberDiff line change
@@ -33,14 +33,14 @@ impl Profile {
3333
for included_profile_str in self.include_profiles.iter() {
3434
if let Some(profile) = registry.profiles.get(included_profile_str) {
3535
for (section, checks) in &profile.sections {
36-
if !self.sections.contains_key(section) {
37-
self.sections.insert(section.clone(), checks.clone());
38-
} else {
36+
if let Some(existing_checks) = self.sections.get_mut(section) {
3937
for check in checks {
40-
if !self.sections[section].contains(check) {
41-
self.sections.get_mut(section).unwrap().push(check.clone());
38+
if !existing_checks.contains(check) {
39+
existing_checks.push(check.clone());
4240
}
4341
}
42+
} else {
43+
self.sections.insert(section.clone(), checks.clone());
4444
}
4545
}
4646
} else {

‎fontspector-checkapi/src/status.rs

+12
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,11 @@ pub enum StatusCode {
88
Pass,
99
Warn,
1010
Fail,
11+
// An Error is when something which returns a Result<> gave us
12+
// an Err - for example a file couldn't be found or couldn't be
13+
// parsed, even though we did our best to check for things. In
14+
// other words, it's something so bad there's no point continuing
15+
// with the check; it's equivalent to a Fontbakery FATAL.
1116
Error,
1217
}
1318

@@ -72,6 +77,13 @@ impl Status {
7277
code: StatusCode::Info,
7378
}
7479
}
80+
pub fn error(s: &str) -> Self {
81+
Self {
82+
message: Some(s.to_string()),
83+
code: StatusCode::Error,
84+
}
85+
}
7586
}
7687

7788
pub type StatusList = Box<dyn Iterator<Item = Status>>;
89+
pub type CheckFnResult = Result<StatusList, String>;

‎fontspector-checkapi/src/testable.rs

+3-5
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,10 @@ impl Testable {
1919
}
2020
}
2121

22-
pub fn basename(&self) -> String {
22+
pub fn basename(&self) -> Option<String> {
2323
std::path::Path::new(&self.filename)
2424
.file_name()
25-
.unwrap()
26-
.to_str()
27-
.unwrap()
28-
.to_string()
25+
.and_then(|x| x.to_str())
26+
.map(|x| x.to_string())
2927
}
3028
}

‎fontspector-cli/src/main.rs

+10-4
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
#![deny(clippy::unwrap_used, clippy::expect_used)]
12
//! Quality control for OpenType fonts
23
use std::collections::HashMap;
34

@@ -53,9 +54,11 @@ fn main() {
5354

5455
// Set up the check registry
5556
let mut registry = Registry::new();
57+
#[allow(clippy::expect_used)] // If this fails, I *want* to panic
5658
Universal
5759
.register(&mut registry)
5860
.expect("Couldn't register universal profile, fontspector bug");
61+
#[allow(clippy::expect_used)] // If this fails, I *want* to panic
5962
GoogleFonts
6063
.register(&mut registry)
6164
.expect("Couldn't register googlefonts profile, fontspector bug");
@@ -78,6 +81,7 @@ fn main() {
7881
.sections
7982
.iter()
8083
.flat_map(|(sectionname, checknames)| {
84+
#[allow(clippy::unwrap_used)] // We previously ensured the check exists in the registry
8185
checknames
8286
.iter()
8387
.map(|checkname| (sectionname.clone(), registry.checks.get(checkname).unwrap()))
@@ -136,13 +140,15 @@ fn main() {
136140
if result.status.code != StatusCode::Fail {
137141
continue;
138142
}
143+
#[allow(clippy::unwrap_used)]
144+
// This is a genuine can't-happen. We put it in the hashmap earlier!
139145
let check = registry.checks.get(&result.check_id).unwrap();
140146
if let Some(fix) = check.hotfix {
141147
if args.hotfix {
142-
if fix(testable) {
143-
println!(" Hotfix applied");
144-
} else {
145-
println!(" Hotfix failed");
148+
match fix(testable) {
149+
Ok(true) => println!(" Hotfix applied"),
150+
Ok(false) => println!(" Hotfix not applied"),
151+
Err(e) => println!(" Hotfix failed: {:}", e),
146152
}
147153
} else {
148154
termimad::print_inline(" This issue can be fixed automatically. Run with `--hotfix` to apply the fix.\n")

‎profile-googlefonts/src/family.rs

+4-4
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@ use std::collections::HashSet;
33

44
use fontspector_checkapi::{prelude::*, FileTypeConvert};
55

6-
fn family_equal_codepoint_coverage(c: &Testable) -> StatusList {
7-
let f = TTF.from_testable(c).expect("Not a TTF file");
6+
fn family_equal_codepoint_coverage(c: &Testable) -> CheckFnResult {
7+
let f = TTF.from_testable(c).ok_or("Not a TTF file")?;
88
let siblings = f.siblings();
99
if siblings.is_empty() {
1010
return return_result(vec![Status::skip("No sibling fonts found")]);
@@ -15,8 +15,8 @@ fn family_equal_codepoint_coverage(c: &Testable) -> StatusList {
1515
let my_codepoints = f.codepoints();
1616
for sibling in siblings {
1717
let their_codepoints = sibling.codepoints();
18-
we_have_they_dont.extend(my_codepoints.difference(&their_codepoints));
19-
they_have_we_dont.extend(their_codepoints.difference(&my_codepoints));
18+
we_have_they_dont.extend(my_codepoints.difference(their_codepoints));
19+
they_have_we_dont.extend(their_codepoints.difference(my_codepoints));
2020
}
2121

2222
if !we_have_they_dont.is_empty() {

‎profile-googlefonts/src/lib.rs

+8-9
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
#![deny(clippy::unwrap_used, clippy::expect_used)]
12
mod family;
23
mod metadata;
34
use family::EQUAL_CODEPOINT_COVERAGE_CHECK;
@@ -11,22 +12,20 @@ impl fontspector_checkapi::Plugin for GoogleFonts {
1112
cr.register_filetype("MDPB", mdpb);
1213
cr.register_check(EQUAL_CODEPOINT_COVERAGE_CHECK);
1314
cr.register_check(VALIDATE_METADATA_PB);
14-
15-
cr.register_profile(
16-
"googlefonts",
17-
Profile::from_toml(
18-
r#"
15+
let profile = Profile::from_toml(
16+
r#"
1917
include_profiles = ["universal"]
2018
[sections]
2119
"Metadata Checks" = [
22-
"com.google.fonts/check/metadata/parses",
20+
"com.google.fonts/check/metadata/parses",
2321
]
2422
"Family Checks" = [
25-
"com.google.fonts/check/family/equal_codepoint_coverage"
23+
"com.google.fonts/check/family/equal_codepoint_coverage"
2624
]
2725
"#,
28-
)
29-
.expect("Couldn't parse profile"),
3026
)
27+
.map_err(|_| "Couldn't parse profile")?;
28+
29+
cr.register_profile("googlefonts", profile)
3130
}
3231
}

0 commit comments

Comments
 (0)