refactor: added new types to form the basis of scoring refactor

hipcheck/Cargo.toml

@@ -63,6 +63,7 @@ url = "2.2.2"
 walkdir = "2.5.0"
 which = { version = "6.0.1", default-features = false }
 xml-rs = "0.8.20"
+indexmap = "2.2.6"
 
 # Windows-specific dependencies
 [target.'cfg(windows)'.dependencies]

hipcheck/src/analysis.rs

@@ -3,6 +3,7 @@
 #[allow(clippy::module_inception)]
 pub mod analysis;
 pub mod report_builder;
+pub mod result;
 pub mod score;
 
 pub use analysis::AnalysisProvider;

hipcheck/src/analysis/analysis.rs

@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: Apache-2.0
 
+use crate::analysis::result::*;
 use crate::config::AttacksConfigQuery;
 use crate::config::CommitConfigQuery;
 use crate::config::FuzzConfigQuery;
@@ -12,7 +13,6 @@ use crate::metric::MetricProvider;
 use crate::report::Concern;
 use crate::report::PrConcern;
 use crate::F64;
-use chrono::Duration;
 use std::collections::HashMap;
 use std::collections::HashSet;
 use std::default::Default;
@@ -33,7 +33,7 @@ pub trait AnalysisProvider:
 	+ PracticesConfigQuery
 {
 	/// Returns result of activity analysis
-	fn activity_analysis(&self) -> Result<Rc<AnalysisReport>>;
+	fn activity_analysis(&self) -> Rc<HCAnalysisReport>;
 
 	/// Returns result of affiliation analysis
 	fn affiliation_analysis(&self) -> Result<Rc<AnalysisReport>>;
@@ -71,13 +71,6 @@ pub trait AnalysisProvider:
 
 #[derive(Debug, Clone, Eq, PartialEq)]
 pub enum AnalysisReport {
-	/// Activity analysis result.
-	Activity {
-		value: u64,
-		threshold: u64,
-		outcome: AnalysisOutcome,
-		concerns: Vec<Concern>,
-	},
 	/// Affiliation analysis result.
 	Affiliation {
 		value: u64,
@@ -186,50 +179,21 @@ impl Display for AnalysisOutcome {
 	}
 }
 
-pub fn activity_analysis(db: &dyn AnalysisProvider) -> Result<Rc<AnalysisReport>> {
-	if db.activity_active() {
-		let results = db.activity_metric();
-		match results {
-			Err(err) => Ok(Rc::new(AnalysisReport::None {
-				outcome: AnalysisOutcome::Error(err),
-			})),
-			Ok(results) => {
-				let value = results.time_since_last_commit.num_weeks();
-				let threshold =
-					Duration::weeks(db.activity_week_count_threshold() as i64).num_weeks();
-				let results_score = score_by_threshold(value, threshold);
-
-				let concerns = Vec::new();
-
-				if results_score == 0 {
-					let msg = format!(
-						"{} weeks inactivity <= {} weeks inactivity",
-						value, threshold
-					);
-					Ok(Rc::new(AnalysisReport::Activity {
-						value: value as u64,
-						threshold: threshold as u64,
-						outcome: AnalysisOutcome::Pass(msg),
-						concerns,
-					}))
-				} else {
-					let msg = format!(
-						"{} weeks inactivity > {} weeks inactivity",
-						value, threshold
-					);
-					Ok(Rc::new(AnalysisReport::Activity {
-						value: value as u64,
-						threshold: threshold as u64,
-						outcome: AnalysisOutcome::Fail(msg),
-						concerns,
-					}))
-				}
-			}
+pub fn activity_analysis(db: &dyn AnalysisProvider) -> Rc<HCAnalysisReport> {
+	let results = db.activity_metric();
+	match results {
+		Err(err) => Rc::new(HCAnalysisReport {
+			outcome: HCAnalysisOutcome::Error(HCAnalysisError::Generic(err)),
+			concerns: vec![],
+		}),
+		Ok(results) => {
+			let value = results.time_since_last_commit.num_weeks() as u64;
+			let hc_value = HCBasicValue::from(value);
+			Rc::new(HCAnalysisReport {
+				outcome: HCAnalysisOutcome::Completed(HCAnalysisValue::Basic(hc_value)),
+				concerns: vec![],
+			})
 		}
-	} else {
-		Ok(Rc::new(AnalysisReport::None {
-			outcome: AnalysisOutcome::Skipped,
-		}))
 	}
 }
 

hipcheck/src/analysis/report_builder.rs

@@ -1,11 +1,13 @@
 // SPDX-License-Identifier: Apache-2.0
 
 use crate::analysis::analysis::AnalysisReport;
+use crate::analysis::result::HCBasicValue;
 use crate::analysis::score::ScoringResults;
 use crate::config::RiskConfigQuery;
 use crate::error::Error;
 use crate::error::Result;
 use crate::hc_error;
+use crate::report::Concern;
 pub use crate::report::*;
 use crate::session::session::Session;
 use crate::source::source::SourceQuery;
@@ -25,35 +27,22 @@ pub fn build_report(session: &Session, scoring: &ScoringResults) -> Result<Repor
 	let mut builder = ReportBuilder::for_session(session);
 
 	// Activity analysis.
-	extract_results(
-		&mut builder,
-		&scoring.results.activity,
-		|builder, output| {
-			match output.as_ref() {
-				AnalysisReport::Activity {
-					value,
-					threshold,
-					concerns,
-					..
-				} => {
-					let analysis = Analysis::activity(*value, *threshold);
-					builder.add_analysis(analysis, concerns.clone())?;
-				}
-				_ => {
-					return Err(hc_error!(
-						"phase name does not match {} analysis",
-						crate::analysis::score::ACTIVITY_PHASE
-					))
-				}
-			}
 
-			Ok(())
-		},
-		|builder, error| {
+	match &scoring.results.activity {
+		Some((Ok(pred), concerns)) => {
+			let HCBasicValue::Unsigned(value) = pred.value else {
+				return Err(hc_error!("activity analysis has a non-u64 value"));
+			};
+			let HCBasicValue::Unsigned(thresh) = pred.threshold else {
+				return Err(hc_error!("activity analysis has a non-u64 value"));
+			};
+			builder.add_analysis(Analysis::activity(value, thresh), concerns.clone())?;
+		}
+		Some((Err(error), _concerns)) => {
 			builder.add_errored_analysis(AnalysisIdent::Activity, error);
-			Ok(())
-		},
-	)?;
+		}
+		None => (),
+	};
 
 	// Affiliation analysis.
 

hipcheck/src/analysis/result.rs

@@ -0,0 +1,170 @@
+// SPDX-License-Identifier: Apache-2.0
+
+use crate::hc_error;
+use crate::report::Concern;
+use crate::Result;
+use crate::F64;
+use std::cmp::Ordering;
+use std::fmt::{self, Display};
+
+/// Represents the enhanced result of a hipcheck analysis. Contains the actual outcome
+/// of the analysis, plus additional meta-information the analysis wants to provide to
+/// Hipcheck core, such as raised concerns.
+#[derive(Debug, Eq, PartialEq)]
+pub struct HCAnalysisReport {
+	pub outcome: HCAnalysisOutcome,
+	pub concerns: Vec<Concern>,
+}
+
+/// Represents the result of a hipcheck analysis. Either the analysis encountered
+/// an error, or it completed and returned a value.
+#[derive(Debug, Clone, Eq, PartialEq)]
+pub enum HCAnalysisOutcome {
+	Error(HCAnalysisError),
+	Completed(HCAnalysisValue),
+}
+
+/// Enumeration of potential errors that a Hipcheck analysis might return. The Generic
+/// variant enables representing errors that aren't covered by other variants.
+#[derive(Debug, Clone, Eq, PartialEq)]
+pub enum HCAnalysisError {
+	Generic(crate::error::Error),
+}
+
+/// A Hipcheck analysis may return a basic or composite value. By splitting the types
+/// into two sub-enums under this one, we can eschew a recursive enum definition and
+/// ensure composite types only have a depth of one.
+#[derive(Debug, Clone, Eq, PartialEq)]
+pub enum HCAnalysisValue {
+	Basic(HCBasicValue),
+	Composite(HCCompositeValue),
+}
+
+/// Basic Hipcheck analysis return types
+#[derive(Debug, Clone, Eq, PartialEq)]
+pub enum HCBasicValue {
+	Integer(i64),
+	Unsigned(u64),
+	Float(F64),
+	Bool(bool),
+	String(String),
+}
+impl From<i64> for HCBasicValue {
+	fn from(value: i64) -> Self {
+		HCBasicValue::Integer(value)
+	}
+}
+impl From<u64> for HCBasicValue {
+	fn from(value: u64) -> Self {
+		HCBasicValue::Unsigned(value)
+	}
+}
+impl From<F64> for HCBasicValue {
+	fn from(value: F64) -> Self {
+		HCBasicValue::Float(value)
+	}
+}
+impl TryFrom<f64> for HCBasicValue {
+	type Error = crate::Error;
+	fn try_from(value: f64) -> Result<HCBasicValue> {
+		let inner = F64::new(value)?;
+		Ok(HCBasicValue::Float(inner))
+	}
+}
+impl From<bool> for HCBasicValue {
+	fn from(value: bool) -> Self {
+		HCBasicValue::Bool(value)
+	}
+}
+impl From<&str> for HCBasicValue {
+	fn from(value: &str) -> Self {
+		HCBasicValue::String(value.to_owned())
+	}
+}
+impl Display for HCBasicValue {
+	fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+		use HCBasicValue::*;
+		match self {
+			Unsigned(u) => u.fmt(f),
+			Integer(i) => i.fmt(f),
+			String(s) => s.fmt(f),
+			Float(fp) => fp.fmt(f),
+			Bool(b) => b.fmt(f),
+		}
+	}
+}
+
+/// Composite Hipcheck analysis return types
+#[derive(Debug, Clone, Eq, PartialEq)]
+pub enum HCCompositeValue {
+	List(Vec<HCBasicValue>),
+	Dict(indexmap::IndexMap<String, HCBasicValue>),
+}
+
+/// The set of possible predicates for deciding if a source passed an analysis.
+pub trait HCPredicate: Display + Clone + Eq + PartialEq {
+	fn pass(&self) -> Result<bool>;
+}
+
+/// This predicate determines analysis pass/fail by whether a returned value was
+/// greater than, less than, or equal to a target value.
+#[derive(Debug, Clone, Eq, PartialEq)]
+pub struct ThresholdPredicate {
+	pub value: HCBasicValue,
+	pub threshold: HCBasicValue,
+	units: String,
+	pub ordering: Ordering,
+}
+impl ThresholdPredicate {
+	pub fn new(
+		value: HCBasicValue,
+		threshold: HCBasicValue,
+		units: Option<String>,
+		ordering: Ordering,
+	) -> Self {
+		ThresholdPredicate {
+			value,
+			threshold,
+			units: units.unwrap_or("".to_owned()),
+			ordering,
+		}
+	}
+}
+
+fn pass_threshold<T: Ord>(a: &T, b: &T, ord: &Ordering) -> bool {
+	a.cmp(b) == *ord
+}
+
+impl HCPredicate for ThresholdPredicate {
+	// @FollowUp - would be nice for this match logic to error at compile time if a new
+	//  HCBasicValue type is added, so developer is reminded to add new variant here
+	fn pass(&self) -> Result<bool> {
+		use HCBasicValue::*;
+		match (&self.value, &self.threshold) {
+			(Integer(a), Integer(b)) => Ok(pass_threshold(a, b, &self.ordering)),
+			(Unsigned(a), Unsigned(b)) => Ok(pass_threshold(a, b, &self.ordering)),
+			(Float(a), Float(b)) => Ok(pass_threshold(a, b, &self.ordering)),
+			(Bool(a), Bool(b)) => Ok(pass_threshold(a, b, &self.ordering)),
+			(String(a), String(b)) => Ok(pass_threshold(a, b, &self.ordering)),
+			(a, b) => Err(hc_error!(
+				"threshold and value are of different types: {:?}, {:?}",
+				a,
+				b
+			)),
+		}
+	}
+}
+impl Display for ThresholdPredicate {
+	fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+		use Ordering::*;
+		// append units. if none, trim() call below will clean up whitespace
+		let val = format!("{} {}", self.value, &self.units);
+		let thr = format!("{} {}", self.threshold, &self.units);
+		let order_str = match &self.ordering {
+			Less => "<",
+			Equal => "==",
+			Greater => ">",
+		};
+		write!(f, "{} {} {}", val.trim(), order_str, thr.trim())
+	}
+}