Skip to content

Commit 5ff0e39

Browse files
simoncozenssimoncozens
committed
Split hotfixing from reporting
1 parent 6480cf0 commit 5ff0e39

File tree

5 files changed

+116
-24
lines changed

5 files changed

+116
-24
lines changed

‎fontspector-checkapi/src/checkresult.rs

+43-1
Original file line numberDiff line numberDiff line change
@@ -2,31 +2,69 @@ use serde::{ser::SerializeStruct, Serialize};
22

33
use crate::{Check, CheckId, Status, StatusCode};
44

5+
#[derive(Debug, Clone, Serialize)]
6+
pub enum FixResult {
7+
/// A fix was available, but not requested
8+
Available,
9+
/// A fix was requested, but no fix was available
10+
Unfixable,
11+
/// A fix was applied
12+
Fixed,
13+
/// The fix failed, for some reason
14+
FixError(String),
15+
}
16+
517
#[derive(Debug, Clone)]
18+
/// The result of a check on one or more font files.
19+
///
20+
/// This struct is used to store the results of a check on one or more font files.
21+
/// A check may return multiple sub-results, as the test checks different aspects
22+
/// of the font file(s). Additionally, fontspector can be used to fix problems
23+
/// found in the font, either at the source or by applying a hotfix. The results
24+
/// of these fixes are stored in the `hotfix_result` and `sourcefix_result` fields.
625
pub struct CheckResult {
26+
/// The ID of the check
727
pub check_id: CheckId,
28+
/// A simple title for the check
829
pub check_name: String,
30+
/// The rationale for the check
931
pub check_rationale: String,
32+
/// The file which was checked; if None, the check was run on all files
1033
pub filename: Option<String>,
34+
/// The section of the profile this check belongs to
1135
pub section: String,
36+
/// The individual results of the check
1237
pub subresults: Vec<Status>,
38+
/// If hotfixing was attempted, the result of the hotfix
39+
pub hotfix_result: Option<FixResult>,
40+
/// If source fixing was attempted, the result of the source fix
41+
pub sourcefix_result: Option<FixResult>,
1342
}
1443

1544
impl Serialize for CheckResult {
1645
fn serialize<S: serde::Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
17-
let mut s = serializer.serialize_struct("CheckResult", 7)?;
46+
let fields =
47+
7 + self.hotfix_result.is_some() as usize + self.sourcefix_result.is_some() as usize;
48+
let mut s = serializer.serialize_struct("CheckResult", fields)?;
1849
s.serialize_field("check_id", &self.check_id)?;
1950
s.serialize_field("check_name", &self.check_name)?;
2051
s.serialize_field("check_rationale", &self.check_rationale)?;
2152
s.serialize_field("filename", &self.filename)?;
2253
s.serialize_field("section", &self.section)?;
2354
s.serialize_field("subresults", &self.subresults)?;
2455
s.serialize_field("worst_status", &self.worst_status())?;
56+
if let Some(hotfix_result) = &self.hotfix_result {
57+
s.serialize_field("hotfix_result", hotfix_result)?;
58+
}
59+
if let Some(sourcefix_result) = &self.sourcefix_result {
60+
s.serialize_field("sourcefix_result", sourcefix_result)?;
61+
}
2562
s.end()
2663
}
2764
}
2865

2966
impl CheckResult {
67+
/// Create a new CheckResult
3068
pub fn new(
3169
check: &Check,
3270
filename: Option<&str>,
@@ -40,9 +78,12 @@ impl CheckResult {
4078
filename: filename.map(|x| x.to_string()),
4179
section: section.to_string(),
4280
subresults,
81+
hotfix_result: None,
82+
sourcefix_result: None,
4383
}
4484
}
4585

86+
/// Get the worst status of all subresults
4687
pub fn worst_status(&self) -> StatusCode {
4788
self.subresults
4889
.iter()
@@ -51,6 +92,7 @@ impl CheckResult {
5192
.unwrap_or(StatusCode::Pass)
5293
}
5394

95+
/// If this check returned some kind of Rust error that we handled
5496
pub fn is_error(&self) -> bool {
5597
self.worst_status() == StatusCode::Error
5698
}

‎fontspector-checkapi/src/lib.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ mod registry;
1010
mod status;
1111
mod testable;
1212
pub use check::{return_result, Check, CheckFlags, CheckId};
13-
pub use checkresult::CheckResult;
13+
pub use checkresult::{CheckResult, FixResult};
1414
pub use context::Context;
1515
pub use filetype::{FileType, FileTypeConvert};
1616
pub use font::{FontCollection, TestFont, TTF};

‎fontspector-cli/src/args.rs

+8-4
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,6 @@ use fontspector_checkapi::StatusCode;
55
#[derive(Parser, Debug)]
66
#[clap(author, version, about, long_about = None)]
77
pub struct Args {
8-
/// Hotfix
9-
#[clap(short, long)]
10-
pub hotfix: bool,
11-
128
/// Plugins to load
139
#[clap(long, value_delimiter = ',')]
1410
pub plugins: Vec<String>,
@@ -69,6 +65,14 @@ pub struct Args {
6965
#[clap(long, help_heading = "Reports")]
7066
pub ghmarkdown: Option<String>,
7167

68+
/// Hotfix found problems in the binaries
69+
#[clap(long, help_heading = "Fix problems")]
70+
pub hotfix: bool,
71+
72+
/// Fix sources
73+
#[clap(long, help_heading = "Fix problems")]
74+
pub fix_sources: bool,
75+
7276
/// Input files
7377
pub inputs: Vec<String>,
7478
}

‎fontspector-cli/src/main.rs

+39-3
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ mod reporters;
66

77
use args::Args;
88
use clap::Parser;
9-
use fontspector_checkapi::{Check, CheckResult, Context, Plugin, Registry, Testable};
9+
use fontspector_checkapi::{Check, CheckResult, Context, FixResult, Plugin, Registry, Testable};
1010
use indicatif::ParallelProgressIterator;
1111
use profile_googlefonts::GoogleFonts;
1212
use profile_universal::Universal;
@@ -135,12 +135,48 @@ fn main() {
135135
);
136136
}
137137

138+
let apply_fixes = |(testable, check, mut result): (&&Testable, &&Check, CheckResult)| {
139+
if args.hotfix {
140+
if let Some(fix) = check.hotfix {
141+
result.hotfix_result = match fix(testable) {
142+
Ok(_) => Some(FixResult::Fixed),
143+
Err(e) => Some(FixResult::FixError(e)),
144+
}
145+
} else {
146+
result.hotfix_result = Some(FixResult::Unfixable);
147+
}
148+
} else if check.hotfix.is_some() {
149+
result.hotfix_result = Some(FixResult::Available);
150+
}
151+
if args.fix_sources {
152+
if let Some(fix) = check.fix_source {
153+
result.sourcefix_result = match fix(testable) {
154+
Ok(_) => Some(FixResult::Fixed),
155+
Err(e) => Some(FixResult::FixError(e)),
156+
}
157+
} else {
158+
result.sourcefix_result = Some(FixResult::Unfixable);
159+
}
160+
} else if check.fix_source.is_some() {
161+
result.sourcefix_result = Some(FixResult::Available);
162+
}
163+
result
164+
};
165+
166+
// Run all the things! Check all the fonts! Fix all the binaries! Fix all the sources!
138167
let results: RunResults = checkorder
139168
.par_iter()
140169
.progress()
141-
.flat_map(|(sectionname, testable, check, context)| {
142-
check.run_one(testable, context, sectionname)
170+
.map(|(sectionname, testable, check, context)| {
171+
(
172+
testable,
173+
check,
174+
check.run_one(testable, context, sectionname),
175+
)
143176
})
177+
.filter(|(_, _, result)| result.is_some())
178+
.map(|(testable, check, result)| (testable, check, result.unwrap()))
179+
.map(apply_fixes)
144180
.collect::<Vec<CheckResult>>()
145181
.into();
146182

‎fontspector-cli/src/reporters/terminal.rs

+25-15
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use super::RunResults;
22
use crate::{reporters::Reporter, Args};
33
use colored::{ColoredString, Colorize};
4-
use fontspector_checkapi::{Registry, StatusCode, Testable};
4+
use fontspector_checkapi::{FixResult, Registry, StatusCode};
55
use itertools::Itertools;
66
use std::{collections::HashMap, path::Path};
77

@@ -31,7 +31,7 @@ fn colored_status(c: StatusCode, s: Option<&str>) -> ColoredString {
3131
}
3232

3333
impl Reporter for TerminalReporter {
34-
fn report(&self, results: &RunResults, args: &Args, registry: &Registry) {
34+
fn report(&self, results: &RunResults, args: &Args, _registry: &Registry) {
3535
let organised_results = results.organize();
3636
for (filename, sectionresults) in organised_results
3737
.iter()
@@ -85,20 +85,30 @@ impl Reporter for TerminalReporter {
8585
));
8686
}
8787
termimad::print_inline(&format!("{:}\n", subresult));
88-
#[allow(clippy::unwrap_used)]
89-
// This is a genuine can't-happen. We put it in the hashmap earlier!
90-
let check = registry.checks.get(&result.check_id).unwrap();
91-
if let Some(fix) = check.hotfix {
92-
if args.hotfix {
93-
match fix(&Testable::new(filename)) {
94-
Ok(true) => println!(" Hotfix applied"),
95-
Ok(false) => println!(" Hotfix not applied"),
96-
Err(e) => println!(" Hotfix failed: {:}", e),
97-
}
98-
} else {
99-
termimad::print_inline(" This issue can be fixed automatically. Run with `--hotfix` to apply the fix.\n")
100-
}
88+
}
89+
match &result.hotfix_result {
90+
Some(FixResult::Available) => {
91+
termimad::print_inline(" This issue can be fixed automatically. Run with `--hotfix` to apply the fix.\n")
92+
}
93+
Some(FixResult::Fixed) => {
94+
termimad::print_inline(" Hotfix applied.\n")
95+
}
96+
Some(FixResult::FixError(e)) => {
97+
termimad::print_inline(&format!(" Hotfix failed: {:}\n", e))
98+
}
99+
_ => {}
100+
}
101+
match &result.sourcefix_result {
102+
Some(FixResult::Available) => {
103+
termimad::print_inline(" This issue can be fixed by modifying the source. Run with `--fix-sources` to apply the fix.\n")
104+
}
105+
Some(FixResult::Fixed) => {
106+
termimad::print_inline(" Source fix applied.\n")
107+
}
108+
Some(FixResult::FixError(e)) => {
109+
termimad::print_inline(&format!(" Source fix failed: {:}\n", e))
101110
}
111+
_ => {}
102112
}
103113
println!("\n");
104114
}

0 commit comments

Comments
 (0)