From f4b77f7d24182488dc4a5fac06cf4f0b06d7b5e4 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 28 Feb 2024 10:10:20 +0100 Subject: [PATCH 1/4] Refactor to prepare for follow-on change --- crates/fj-core/src/validation/validation_check.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/fj-core/src/validation/validation_check.rs b/crates/fj-core/src/validation/validation_check.rs index 5e30c92a5..a2551f505 100644 --- a/crates/fj-core/src/validation/validation_check.rs +++ b/crates/fj-core/src/validation/validation_check.rs @@ -15,8 +15,8 @@ pub trait ValidationCheck { /// This method is designed for convenience over flexibility (it is intended /// for use in unit tests), and thus always uses the default configuration. fn check_and_return_first_error(&self) -> Result<(), T> { - let errors = - self.check(&ValidationConfig::default()).collect::>(); + let config = ValidationConfig::default(); + let errors = self.check(&config).collect::>(); if let Some(err) = errors.into_iter().next() { return Err(err); From d26a2550e28288d4e01c3d8997f0869c8b171bb8 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 28 Feb 2024 10:10:53 +0100 Subject: [PATCH 2/4] Remove unnecessary allocation --- crates/fj-core/src/validation/validation_check.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/fj-core/src/validation/validation_check.rs b/crates/fj-core/src/validation/validation_check.rs index a2551f505..cea836366 100644 --- a/crates/fj-core/src/validation/validation_check.rs +++ b/crates/fj-core/src/validation/validation_check.rs @@ -16,9 +16,9 @@ pub trait ValidationCheck { /// for use in unit tests), and thus always uses the default configuration. fn check_and_return_first_error(&self) -> Result<(), T> { let config = ValidationConfig::default(); - let errors = self.check(&config).collect::>(); + let mut errors = self.check(&config); - if let Some(err) = errors.into_iter().next() { + if let Some(err) = errors.next() { return Err(err); } From cdc0ef70f22927542aebf77b9952c8962acd7f82 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 28 Feb 2024 10:12:08 +0100 Subject: [PATCH 3/4] Return error from `check_and_expect_one_error` --- crates/fj-core/src/validation/validation_check.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/fj-core/src/validation/validation_check.rs b/crates/fj-core/src/validation/validation_check.rs index cea836366..ee8b09faf 100644 --- a/crates/fj-core/src/validation/validation_check.rs +++ b/crates/fj-core/src/validation/validation_check.rs @@ -29,14 +29,14 @@ pub trait ValidationCheck { /// /// This method is designed for convenience over flexibility (it is intended /// for use in unit tests), and thus always uses the default configuration. - fn check_and_expect_one_error(&self) + fn check_and_expect_one_error(&self) -> T where T: Display, { let config = ValidationConfig::default(); let mut errors = self.check(&config).peekable(); - errors + let err = errors .next() .expect("Expected one validation error; none found"); @@ -49,5 +49,7 @@ pub trait ValidationCheck { panic!("Expected only one validation error") } + + err } } From 37688be12d6ea893f3b80450e6bd95a0af0ccafc Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 28 Feb 2024 10:18:16 +0100 Subject: [PATCH 4/4] Implement `ValidationCheck` for the check The way it was previously, fully qualified method call syntax would have been required to run a validation check. This wasn't visible in the code so far, as there only is one validation check currently. This change makes running validation checks a bit more convenient (compared to what it would have needed to be, not what's currently in the code). It preserves the discoverability, as the implementations are still listed in the documentation of the object type. --- crates/fj-core/src/validate/cycle.rs | 9 +++++++-- .../validation/checks/half_edge_connection.rs | 14 ++++++++------ .../fj-core/src/validation/validation_check.rs | 17 ++++++++++------- 3 files changed, 25 insertions(+), 15 deletions(-) diff --git a/crates/fj-core/src/validate/cycle.rs b/crates/fj-core/src/validate/cycle.rs index a1d628605..dc9b125ec 100644 --- a/crates/fj-core/src/validate/cycle.rs +++ b/crates/fj-core/src/validate/cycle.rs @@ -1,6 +1,9 @@ use crate::{ objects::Cycle, - validation::{ValidationCheck, ValidationConfig, ValidationError}, + validation::{ + checks::AdjacentHalfEdgesNotConnected, ValidationCheck, + ValidationConfig, ValidationError, + }, }; use super::Validate; @@ -11,6 +14,8 @@ impl Validate for Cycle { config: &ValidationConfig, errors: &mut Vec, ) { - errors.extend(self.check(config).map(Into::into)); + errors.extend( + AdjacentHalfEdgesNotConnected::check(self, config).map(Into::into), + ); } } diff --git a/crates/fj-core/src/validation/checks/half_edge_connection.rs b/crates/fj-core/src/validation/checks/half_edge_connection.rs index 296f5ac86..04f45b2e9 100644 --- a/crates/fj-core/src/validation/checks/half_edge_connection.rs +++ b/crates/fj-core/src/validation/checks/half_edge_connection.rs @@ -37,12 +37,12 @@ pub struct AdjacentHalfEdgesNotConnected { pub unconnected_half_edges: [Handle; 2], } -impl ValidationCheck for Cycle { +impl ValidationCheck for AdjacentHalfEdgesNotConnected { fn check( - &self, + object: &Cycle, config: &ValidationConfig, - ) -> impl Iterator { - self.half_edges().pairs().filter_map(|(first, second)| { + ) -> impl Iterator { + object.half_edges().pairs().filter_map(|(first, second)| { let end_pos_of_first_half_edge = { let [_, end] = first.boundary().inner; first.path().point_from_path_coords(end) @@ -80,12 +80,14 @@ mod tests { Core, }; + use super::AdjacentHalfEdgesNotConnected; + #[test] fn adjacent_half_edges_connected() -> anyhow::Result<()> { let mut core = Core::new(); let valid = Cycle::polygon([[0., 0.], [1., 0.], [1., 1.]], &mut core); - valid.check_and_return_first_error()?; + AdjacentHalfEdgesNotConnected::check_and_return_first_error(&valid)?; let invalid = valid.update_half_edge( valid.half_edges().first(), @@ -94,7 +96,7 @@ mod tests { }, &mut core, ); - invalid.check_and_expect_one_error(); + AdjacentHalfEdgesNotConnected::check_and_expect_one_error(&invalid); Ok(()) } diff --git a/crates/fj-core/src/validation/validation_check.rs b/crates/fj-core/src/validation/validation_check.rs index ee8b09faf..9e75b1aef 100644 --- a/crates/fj-core/src/validation/validation_check.rs +++ b/crates/fj-core/src/validation/validation_check.rs @@ -6,17 +6,20 @@ use super::ValidationConfig; /// /// This trait is implemented once per validation check and object it applies /// to. `Self` is the object, while `T` identifies the validation check. -pub trait ValidationCheck { +pub trait ValidationCheck: Sized { /// Run the validation check on the implementing object - fn check(&self, config: &ValidationConfig) -> impl Iterator; + fn check( + object: &T, + config: &ValidationConfig, + ) -> impl Iterator; /// Convenience method to run the check return the first error /// /// This method is designed for convenience over flexibility (it is intended /// for use in unit tests), and thus always uses the default configuration. - fn check_and_return_first_error(&self) -> Result<(), T> { + fn check_and_return_first_error(object: &T) -> Result<(), Self> { let config = ValidationConfig::default(); - let mut errors = self.check(&config); + let mut errors = Self::check(object, &config); if let Some(err) = errors.next() { return Err(err); @@ -29,12 +32,12 @@ pub trait ValidationCheck { /// /// This method is designed for convenience over flexibility (it is intended /// for use in unit tests), and thus always uses the default configuration. - fn check_and_expect_one_error(&self) -> T + fn check_and_expect_one_error(object: &T) -> Self where - T: Display, + Self: Display, { let config = ValidationConfig::default(); - let mut errors = self.check(&config).peekable(); + let mut errors = Self::check(object, &config).peekable(); let err = errors .next()