From fbe3c76ab780dbf67ab672d340e006db8a300f52 Mon Sep 17 00:00:00 2001
From: Tom Alexander <tom@fizz.buzz>
Date: Fri, 6 Oct 2023 16:32:49 -0400
Subject: [PATCH] Cleanup.

---
 src/compare/compare_field.rs |  2 +-
 src/compare/diff.rs          | 39 +++++++++++++++++---------------
 src/compare/macros.rs        | 44 +++++++++++++++++++++++++-----------
 3 files changed, 53 insertions(+), 32 deletions(-)

diff --git a/src/compare/compare_field.rs b/src/compare/compare_field.rs
index 4a860f78..5a263237 100644
--- a/src/compare/compare_field.rs
+++ b/src/compare/compare_field.rs
@@ -15,7 +15,7 @@ pub(crate) enum EmacsField<'s> {
 ///
 /// This is for when you want to acknowledge that a field exists in the emacs token, but you do not have any validation for it when using the compare_properties!() macro. Ideally, this should be kept to a minimum since this represents untested values.
 #[allow(dead_code)]
-pub(crate) fn impl_compare_noop<'b, 's, 'x, R, RG>(
+pub(crate) fn compare_noop<'b, 's, 'x, R, RG>(
     _emacs: &'b Token<'s>,
     _rust_node: R,
     _emacs_field: &'x str,
diff --git a/src/compare/diff.rs b/src/compare/diff.rs
index ac638627..cf802f8c 100644
--- a/src/compare/diff.rs
+++ b/src/compare/diff.rs
@@ -3,9 +3,7 @@ use std::borrow::Cow;
 use std::collections::BTreeSet;
 use std::collections::HashSet;
 
-use super::compare_field::compare_identity;
 use super::compare_field::compare_property_quoted_string;
-use super::compare_field::impl_compare_noop;
 use super::elisp_fact::ElispFact;
 use super::elisp_fact::GetElispFact;
 use super::sexp::unquote;
@@ -2607,9 +2605,14 @@ fn compare_italic<'b, 's>(
     rust: &'b Italic<'s>,
 ) -> Result<DiffEntry<'b, 's>, Box<dyn std::error::Error>> {
     let children = emacs.as_list()?;
-    let this_status = DiffStatus::Good;
+    let mut this_status = DiffStatus::Good;
     let mut child_status = Vec::new();
-    let message = None;
+    let mut message = None;
+
+    if let Some((new_status, new_message)) = compare_properties!(emacs)? {
+        this_status = new_status;
+        message = new_message;
+    }
 
     for (emacs_child, rust_child) in children.iter().skip(2).zip(rust.children.iter()) {
         child_status.push(compare_ast_node(source, emacs_child, rust_child.into())?);
@@ -2632,9 +2635,14 @@ fn compare_underline<'b, 's>(
     rust: &'b Underline<'s>,
 ) -> Result<DiffEntry<'b, 's>, Box<dyn std::error::Error>> {
     let children = emacs.as_list()?;
-    let this_status = DiffStatus::Good;
+    let mut this_status = DiffStatus::Good;
     let mut child_status = Vec::new();
-    let message = None;
+    let mut message = None;
+
+    if let Some((new_status, new_message)) = compare_properties!(emacs)? {
+        this_status = new_status;
+        message = new_message;
+    }
 
     for (emacs_child, rust_child) in children.iter().skip(2).zip(rust.children.iter()) {
         child_status.push(compare_ast_node(source, emacs_child, rust_child.into())?);
@@ -2666,16 +2674,6 @@ fn compare_verbatim<'b, 's>(
             EmacsField::Required(":value"),
             |r| Some(r.contents),
             compare_property_quoted_string
-        ),
-        (
-            EmacsField::Required(":value"),
-            |r| Some(r.contents),
-            compare_property_quoted_string
-        ),
-        (
-            EmacsField::Required(":foo"),
-            compare_identity,
-            impl_compare_noop
         )
     )? {
         this_status = new_status;
@@ -2731,9 +2729,14 @@ fn compare_strike_through<'b, 's>(
     rust: &'b StrikeThrough<'s>,
 ) -> Result<DiffEntry<'b, 's>, Box<dyn std::error::Error>> {
     let children = emacs.as_list()?;
-    let this_status = DiffStatus::Good;
+    let mut this_status = DiffStatus::Good;
     let mut child_status = Vec::new();
-    let message = None;
+    let mut message = None;
+
+    if let Some((new_status, new_message)) = compare_properties!(emacs)? {
+        this_status = new_status;
+        message = new_message;
+    }
 
     for (emacs_child, rust_child) in children.iter().skip(2).zip(rust.children.iter()) {
         child_status.push(compare_ast_node(source, emacs_child, rust_child.into())?);
diff --git a/src/compare/macros.rs b/src/compare/macros.rs
index 2f2947f1..c71ce9d5 100644
--- a/src/compare/macros.rs
+++ b/src/compare/macros.rs
@@ -1,16 +1,34 @@
-macro_rules! compare_noop {
-    ($($field:expr),+) => {
-        $(
-        EmacsField::Required(":foo"),
-        compare_identity,
-        impl_compare_noop
-),+
-    };
-}
-
-pub(crate) use compare_noop;
-
-/// Create iterators for ast nodes where it only has to iterate over children
+/// Assert only the listed properties exist on the Emacs AST node and compare their values to the values on the rust AST node.
+///
+/// Example invocation:
+/// ```
+///     if let Some((new_status, new_message)) = compare_properties!(
+///         emacs,
+///         rust,
+///         (
+///             EmacsField::Required(":key"),
+///             |r| Some(r.key),
+///             compare_property_quoted_string
+///         ),
+///         (
+///             EmacsField::Required(":value"),
+///             |r| Some(r.contents),
+///             compare_property_quoted_string
+///         ),
+///         (EmacsField::Required(":pre-blank"), compare_identity, compare_noop)
+///     )? {
+///         this_status = new_status;
+///         message = new_message;
+///     }
+/// ```
+///
+/// Or if the node has no properties aside from :standard-properties, we can still assert that the node has no unexpected properties:
+/// ```
+///     if let Some((new_status, new_message)) = compare_properties!(emacs)? {
+///         this_status = new_status;
+///         message = new_message;
+///     }
+/// ```
 macro_rules! compare_properties {
     ($emacs:expr, $rust:expr, $(($emacs_field:expr, $rust_value_getter:expr, $compare_fn: expr)),+) => {
         {