Merge pull request #5 from felipesanches/rationales_not_optional

Rationale and proposal fields are not optional

Author: simoncozens

fontspector-checkapi/src/check.rs

@@ -8,8 +8,8 @@ pub type CheckId = String;
 pub struct Check<'a> {
     pub id: &'a str,
     pub title: &'a str,
-    pub rationale: Option<&'a str>,
-    pub proposal: Option<&'a str>,
+    pub rationale: &'a str,
+    pub proposal: &'a str,
     pub check_one: Option<&'a dyn Fn(&Testable) -> CheckFnResult>,
     pub check_all: Option<&'a dyn Fn(&FontCollection) -> CheckFnResult>,
     pub hotfix: Option<&'a dyn Fn(&Testable) -> FixFnResult>,
@@ -24,7 +24,7 @@ pub struct CheckResult {
     pub status: Status,
     pub check_id: CheckId,
     pub check_name: String,
-    pub check_rationale: Option<String>,
+    pub check_rationale: String,
     pub filename: Option<String>,
 }
 
@@ -41,7 +41,7 @@ impl<'a> Check<'a> {
             status,
             check_id: self.id.to_string(),
             check_name: self.title.to_string(),
-            check_rationale: self.rationale.map(|x| x.to_string()),
+            check_rationale: self.rationale.to_string(),
             filename: file.map(|x| x.filename.clone()),
         }
     }

fontspector-cli/src/main.rs

@@ -131,10 +131,8 @@ fn main() {
                 if args.verbose > 1 {
                     println!("   {:}", result.check_name);
                 }
-                if let Some(rationale) = &result.check_rationale {
-                    if args.verbose > 1 {
-                        termimad::print_inline(&format!("Rationale:\n\n```\n{}\n```\n", rationale));
-                    }
+                if args.verbose > 1 {
+                    termimad::print_inline(&format!("Rationale:\n\n```\n{}\n```\n", result.check_rationale));
                 }
                 termimad::print_inline(&format!("**{:}**", result.status));
                 if result.status.code != StatusCode::Fail {

profile-googlefonts/src/family.rs

@@ -40,17 +40,15 @@ fn family_equal_codepoint_coverage(c: &Testable) -> CheckFnResult {
     return_result(problems)
 }
 
-pub const EQUAL_CODEPOINT_COVERAGE_CHECK: Check = Check {
+pub const CHECK_FAMILY_EQUAL_CODEPOINT_COVERAGE: Check = Check {
     id: "com.google.fonts/check/family/equal_codepoint_coverage",
     title: "Fonts have equal codepoint coverage?",
-    rationale: Some(
-        "For a given family, all fonts must have the same codepoint coverage.
-        This is because we want to avoid the situation where, for example,
-        a character is present in a regular font but missing in the italic style;
-        turning on italic would cause the character to be rendered either as a
-        fake italic (auto-slanted) or to show tofu.",
-    ),
-    proposal: Some("https://github.com/fonttools/fontbakery/issues/4180"),
+    rationale: "For a given family, all fonts must have the same codepoint coverage.
+                This is because we want to avoid the situation where, for example,
+                a character is present in a regular font but missing in the italic
+                style; turning on italic would cause the character to be rendered
+                either as a fake italic (auto-slanted) or to show tofu.",
+    proposal: "https://github.com/fonttools/fontbakery/issues/4180",
     check_one: Some(&family_equal_codepoint_coverage),
     check_all: None,
     applies_to: "TTF",

profile-googlefonts/src/lib.rs

@@ -1,17 +1,17 @@
 #![deny(clippy::unwrap_used, clippy::expect_used)]
 mod family;
 mod metadata;
-use family::EQUAL_CODEPOINT_COVERAGE_CHECK;
+use family::CHECK_FAMILY_EQUAL_CODEPOINT_COVERAGE;
 use fontspector_checkapi::prelude::*;
-use metadata::VALIDATE_METADATA_PB;
+use metadata::CHECK_METADATA_PARSES;
 
 pub struct GoogleFonts;
 impl fontspector_checkapi::Plugin for GoogleFonts {
     fn register(&self, cr: &mut Registry) -> Result<(), String> {
         let mdpb = FileType::new("METADATA.pb");
         cr.register_filetype("MDPB", mdpb);
-        cr.register_check(EQUAL_CODEPOINT_COVERAGE_CHECK);
-        cr.register_check(VALIDATE_METADATA_PB);
+        cr.register_check(CHECK_FAMILY_EQUAL_CODEPOINT_COVERAGE);
+        cr.register_check(CHECK_METADATA_PARSES);
         let profile = Profile::from_toml(
             r#"
 include_profiles = ["universal"]

profile-googlefonts/src/metadata.rs

@@ -43,11 +43,14 @@ fn validate_metadatapb(c: &Testable) -> CheckFnResult {
     }
 }
 
-pub const VALIDATE_METADATA_PB: Check = Check {
+pub const CHECK_METADATA_PARSES: Check = Check {
     id: "com.google.fonts/check/metadata/parses",
-    title: "Check METADATA.pb parse correctly",
-    rationale: None,
-    proposal: None,
+    title: "Check METADATA.pb parses correctly",
+    rationale: "
+        The purpose of this check is to ensure that the METADATA.pb file is not
+        malformed.
+    ",
+    proposal: "https://github.com/fonttools/fontbakery/issues/2248",
     check_all: None,
     check_one: Some(&validate_metadatapb),
     applies_to: "MDPB",

profile-testplugin/src/lib.rs

@@ -19,8 +19,8 @@ fn validate_toml(c: &Testable) -> CheckFnResult {
 pub const SAY_HELLO: Check = Check {
     id: "com.google.fonts/check/test/say_hello",
     title: "Check that the plugin protocol is working",
-    rationale: None,
-    proposal: None,
+    rationale: "This check is part of the example of how to create plugins.",
+    proposal: "https://github.com/simoncozens/fontspector/commit/5fdf9750991176c8e2776557ce6c17c642c24a73",
     check_all: None,
     check_one: Some(&say_hello),
     applies_to: "TTF",
@@ -31,8 +31,8 @@ pub const SAY_HELLO: Check = Check {
 pub const VALIDATE_TOML: Check = Check {
     id: "com.google.fonts/check/test/validate_toml",
     title: "Check that the filetype plugin protocol is working",
-    rationale: None,
-    proposal: None,
+    rationale: "This check is part of the example of how to create plugins.",
+    proposal: "https://github.com/simoncozens/fontspector/commit/5fdf9750991176c8e2776557ce6c17c642c24a73",
     check_all: None,
     check_one: Some(&validate_toml),
     applies_to: "TOML",

profile-universal/src/checks/mod.rs

@@ -2,7 +2,7 @@ mod bold_italic_unique;
 mod name_trailing_spaces;
 mod required_tables;
 mod unwanted_tables;
-// pub use bold_italic_unique::BOLD_ITALIC_UNIQUE_CHECK;
-pub use name_trailing_spaces::NAME_TRAILING_SPACES_CHECK;
-pub use required_tables::REQUIRED_TABLES_CHECK;
-pub use unwanted_tables::UNWANTED_TABLES_CHECK;
+// pub use bold_italic_unique::CHECK_BOLD_ITALIC_UNIQUE;
+pub use name_trailing_spaces::CHECK_NAME_TRAILING_SPACES;
+pub use required_tables::CHECK_REQUIRED_TABLES;
+pub use unwanted_tables::CHECK_UNWANTED_TABLES;

profile-universal/src/checks/name_trailing_spaces.rs

@@ -30,11 +30,14 @@ fn fix_trailing_spaces(_f: &Testable) -> FixFnResult {
     Ok(false)
 }
 
-pub const NAME_TRAILING_SPACES_CHECK: Check = Check {
+pub const CHECK_NAME_TRAILING_SPACES: Check = Check {
     id: "com.google.fonts/check/name/trailing_spaces",
     title: "Name table records must not have trailing spaces.",
-    rationale: None,
-    proposal: Some("https://github.com/googlefonts/fontbakery/issues/2417"),
+    rationale: "This check ensures that no entries in the name table end in spaces;
+                trailing spaces, particularly in font names, can be confusing to users.
+                In most cases this can be fixed by removing trailing spaces from the
+                metadata fields in the font editor.",
+    proposal: "https://github.com/googlefonts/fontbakery/issues/2417",
     check_one: Some(&name_trailing_spaces),
     check_all: None,
     applies_to: "TTF",

profile-universal/src/checks/required_tables.rs

@@ -82,11 +82,10 @@ fn required_tables(t: &Testable) -> CheckFnResult {
     return_result(problems)
 }
 
-pub const REQUIRED_TABLES_CHECK: Check = Check {
+pub const CHECK_REQUIRED_TABLES: Check = Check {
     id: "com.google.fonts/check/required_tables",
     title: "Font contains all required tables?",
-    rationale: Some(
-        "
+    rationale: "
         According to the OpenType spec
         https://docs.microsoft.com/en-us/typography/opentype/spec/otff#required-tables
 
@@ -116,8 +115,7 @@ pub const REQUIRED_TABLES_CHECK: Check = Check {
         - A gasp table is necessary if a designer wants to influence the sizes
           at which grayscaling is used under Windows. Etc.
     ",
-    ),
-    proposal: Some("legacy:check/052"),
+    proposal: "legacy:check/052",
     check_one: Some(&required_tables),
     check_all: None,
     applies_to: "TTF",

profile-universal/src/checks/unwanted_tables.rs

@@ -49,11 +49,11 @@ fn delete_unwanted_tables(t: &Testable) -> FixFnResult {
     Ok(true)
 }
 
-pub const UNWANTED_TABLES_CHECK: Check = Check {
+pub const CHECK_UNWANTED_TABLES: Check = Check {
     id: "com.google.fonts/check/unwanted_tables",
     title: "Are there unwanted tables?",
-    rationale: Some("Some font editors store source data in their own SFNT tables, and these can sometimes sneak into final release files, which should only have OpenType spec tables."),
-    proposal: Some("legacy:check/053"),
+    rationale: "Some font editors store source data in their own SFNT tables, and these can sometimes sneak into final release files, which should only have OpenType spec tables.",
+    proposal: "legacy:check/053",
     check_one: Some(&unwanted_tables),
     check_all: None,
     applies_to: "TTF",

profile-universal/src/lib.rs

@@ -6,9 +6,9 @@ pub struct Universal;
 
 impl fontspector_checkapi::Plugin for Universal {
     fn register(&self, cr: &mut Registry) -> Result<(), String> {
-        cr.register_check(checks::NAME_TRAILING_SPACES_CHECK);
-        cr.register_check(checks::UNWANTED_TABLES_CHECK);
-        cr.register_check(checks::REQUIRED_TABLES_CHECK);
+        cr.register_check(checks::CHECK_NAME_TRAILING_SPACES);
+        cr.register_check(checks::CHECK_UNWANTED_TABLES);
+        cr.register_check(checks::CHECK_REQUIRED_TABLES);
         let profile = Profile::from_toml(
             r#"
 [sections]