Tidy up checkorder madness, make siblings work in WASM

simoncozens · simoncozens · commit da1d14229143 · 2024-08-21T17:13:54.000+01:00
diff --git a/fontspector-checkapi/src/context.rs b/fontspector-checkapi/src/context.rs
@@ -6,3 +6,26 @@ pub struct Context {
     pub network_timeout: Option<u64>,
     pub configuration: Map<String, Value>,
 }
+
+impl Context {
+    /// Extract a specialized context for a specific check using a configuration map
+    ///
+    /// For example, if the check is `googlefonts/has_metadata`, the configuration map
+    /// will be searched for a key `googlefonts/has_metadata` and the value will be used
+    /// as the configuration for this check.
+    pub fn specialize(
+        &self,
+        checkname: &str,
+        configuration: &Map<String, serde_json::Value>,
+    ) -> Self {
+        Context {
+            skip_network: self.skip_network,
+            network_timeout: self.network_timeout,
+            configuration: configuration
+                .get(checkname)
+                .and_then(|x| x.as_object())
+                .cloned()
+                .unwrap_or_default(),
+        }
+    }
+}
diff --git a/fontspector-checkapi/src/profile.rs b/fontspector-checkapi/src/profile.rs
@@ -1,6 +1,7 @@
 use serde::{Deserialize, Serialize};
+use serde_json::Map;
 
-use crate::{CheckId, Registry, StatusCode};
+use crate::{Check, CheckId, Context, Registry, StatusCode, TestableType};
 use std::collections::HashMap;
 
 #[derive(Serialize, Deserialize)]
@@ -70,4 +71,59 @@ impl Profile {
         }
         Ok(())
     }
+
+    pub fn check_order<'t, 'r>(
+        &self,
+        include_checks: &Option<Vec<String>>,
+        exclude_checks: &Option<Vec<String>>,
+        registry: &'r Registry<'r>,
+        general_context: Context,
+        configuration: Map<String, serde_json::Value>,
+        testables: &'t Vec<TestableType>,
+    ) -> Vec<(String, &'t TestableType<'t>, &'r Check<'r>, Context)> {
+        self.sections
+            .iter()
+            .flat_map(|(sectionname, checknames)| {
+                #[allow(clippy::unwrap_used)]
+                // We previously ensured the check exists in the registry
+                checknames
+                    .iter()
+                    .filter(|checkname| {
+                        included_excluded(checkname, include_checks, exclude_checks)
+                    })
+                    .map(|checkname| {
+                        (
+                            sectionname.clone(),
+                            registry.checks.get(checkname).unwrap(),
+                            general_context.specialize(checkname, &configuration),
+                        )
+                    })
+            })
+            .flat_map(|(sectionname, check, context): (String, &Check, Context)| {
+                testables
+                    .iter()
+                    .filter(|testable| check.applies(testable, registry))
+                    .map(move |testable| (sectionname.clone(), testable, check, context.clone()))
+            })
+            .collect()
+    }
+}
+
+/// Filter out checks that don't apply
+fn included_excluded(
+    checkname: &str,
+    include_checks: &Option<Vec<String>>,
+    exclude_checks: &Option<Vec<String>>,
+) -> bool {
+    if let Some(checkids) = &include_checks {
+        if !checkids.iter().any(|id| checkname.contains(id)) {
+            return false;
+        }
+    }
+    if let Some(exclude_checkids) = &exclude_checks {
+        if exclude_checkids.iter().any(|id| checkname.contains(id)) {
+            return false;
+        }
+    }
+    true
 }
diff --git a/fontspector-checkapi/src/testable.rs b/fontspector-checkapi/src/testable.rs
@@ -94,6 +94,10 @@ impl TestableCollection {
         })
     }
 
+    pub fn from_testables(testables: Vec<Testable>) -> Self {
+        Self { testables }
+    }
+
     pub fn iter(&self) -> impl Iterator<Item = &Testable> {
         self.testables.iter()
     }
diff --git a/fontspector-cli/src/main.rs b/fontspector-cli/src/main.rs
@@ -21,21 +21,6 @@ use reporters::{
 };
 use serde_json::Map;
 
-/// Filter out checks that don't apply
-fn included_excluded(checkname: &str, args: &Args) -> bool {
-    if let Some(checkids) = &args.checkid {
-        if !checkids.iter().any(|id| checkname.contains(id)) {
-            return false;
-        }
-    }
-    if let Some(exclude_checkids) = &args.exclude_checkid {
-        if exclude_checkids.iter().any(|id| checkname.contains(id)) {
-            return false;
-        }
-    }
-    true
-}
-
 fn main() {
     // Command line handling
     let args = Args::parse();
@@ -112,29 +97,18 @@ fn main() {
     let configuration: Map<String, serde_json::Value> = load_configuration(&args);
 
     // Establish a check order
-    let checkorder: Vec<(String, &TestableType, &Check, Context)> = profile
-        .sections
-        .iter()
-        .flat_map(|(sectionname, checknames)| {
-            #[allow(clippy::unwrap_used)] // We previously ensured the check exists in the registry
-            checknames
-                .iter()
-                .filter(|checkname| included_excluded(checkname, &args))
-                .map(|checkname| {
-                    (
-                        sectionname.clone(),
-                        registry.checks.get(checkname).unwrap(),
-                        context_for(checkname, &args, &configuration),
-                    )
-                })
-        })
-        .flat_map(|(sectionname, check, context): (String, &Check, Context)| {
-            testables
-                .iter()
-                .filter(|testable| check.applies(testable, &registry))
-                .map(move |testable| (sectionname.clone(), testable, check, context.clone()))
-        })
-        .collect();
+    let checkorder: Vec<(String, &TestableType, &Check, Context)> = profile.check_order(
+        &args.checkid,
+        &args.exclude_checkid,
+        &registry,
+        Context {
+            skip_network: args.skip_network,
+            network_timeout: Some(10), // XXX
+            configuration: Map::new(),
+        },
+        configuration,
+        &testables,
+    );
 
     // The testables are the collection object plus the files; only count the files.
     let count_of_files = testables.iter().filter(|x| x.is_single()).count();
@@ -246,19 +220,3 @@ fn load_configuration(args: &Args) -> Map<String, serde_json::Value> {
         })
         .unwrap_or_default()
 }
-
-fn context_for(
-    checkname: &str,
-    args: &Args,
-    configuration: &Map<String, serde_json::Value>,
-) -> Context {
-    Context {
-        skip_network: args.skip_network,
-        network_timeout: args.timeout,
-        configuration: configuration
-            .get(checkname)
-            .and_then(|x| x.as_object())
-            .cloned()
-            .unwrap_or_default(),
-    }
-}
diff --git a/fontspector-web/src/lib.rs b/fontspector-web/src/lib.rs
@@ -1,7 +1,9 @@
 use js_sys::{Reflect, Uint8Array};
 use wasm_bindgen::prelude::*;
 extern crate console_error_panic_hook;
-use fontspector_checkapi::{Check, CheckResult, Context, Plugin, Registry, Testable};
+use fontspector_checkapi::{
+    Check, CheckResult, Context, Plugin, Registry, Testable, TestableCollection, TestableType,
+};
 use profile_googlefonts::GoogleFonts;
 use profile_universal::Universal;
 
@@ -32,45 +34,29 @@ pub fn check_fonts(fonts: &JsValue) -> Result<String, JsValue> {
             }
         })
         .collect();
+    let collection = TestableCollection::from_testables(testables);
+
     let profile = registry.get_profile("googlefonts").unwrap();
     let context = Context {
         skip_network: true,
         network_timeout: None,
         configuration: serde_json::Map::new(),
     };
+    let all_testables = collection.collection_and_files().collect();
 
-    let checkorder: Vec<(String, &Testable, &Check, Context)> = profile
-        .sections
-        .iter()
-        .flat_map(|(sectionname, checknames)| {
-            #[allow(clippy::unwrap_used)] // We previously ensured the check exists in the registry
-            checknames
-                .iter()
-                // .filter(|checkname| included_excluded(checkname, &args))
-                .map(|checkname| {
-                    (
-                        sectionname.clone(),
-                        registry.checks.get(checkname).unwrap(),
-                        context.clone(),
-                    )
-                })
-        })
-        .flat_map(|(sectionname, check, context): (String, &Check, Context)| {
-            testables
-                .iter()
-                .filter(|testable| check.applies(testable, &registry))
-                .map(move |testable| (sectionname.clone(), testable, check, context.clone()))
-        })
-        .collect();
+    let checkorder: Vec<(String, &TestableType, &Check, Context)> = profile.check_order(
+        &None,
+        &None,
+        &registry,
+        context,
+        serde_json::Map::new(),
+        &all_testables,
+    );
 
     let results: Vec<CheckResult> = checkorder
         .iter()
         .map(|(sectionname, testable, check, context)| {
-            (
-                testable,
-                check,
-                check.run_one(testable, context, sectionname),
-            )
+            (testable, check, check.run(testable, context, sectionname))
         })
         .flat_map(|(_, _, result)| result)
         .collect();

Original file line number	Diff line number	Diff line change
`@@ -94,6 +94,10 @@ impl TestableCollection {`
`94`	`94`	`})`
`95`	`95`	`}`
`96`	`96`
	`97`	`+ pub fn from_testables(testables: Vec<Testable>) -> Self {`
	`98`	`+ Self { testables }`
	`99`	`+ }`
	`100`	`+`
`97`	`101`	`pub fn iter(&self) -> impl Iterator<Item = &Testable> {`
`98`	`102`	`self.testables.iter()`
`99`	`103`	`}`