From 5363324bbf5de6728279d270500d67f708d58a84 Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Wed, 11 Oct 2023 17:37:39 -0400 Subject: [PATCH 01/10] Add a test showing we are not handling optional values properly. --- org_mode_samples/affiliated_keyword/optional_value.org | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 org_mode_samples/affiliated_keyword/optional_value.org diff --git a/org_mode_samples/affiliated_keyword/optional_value.org b/org_mode_samples/affiliated_keyword/optional_value.org new file mode 100644 index 00000000..99ebc24e --- /dev/null +++ b/org_mode_samples/affiliated_keyword/optional_value.org @@ -0,0 +1,3 @@ +#+CAPTION[foo]: *bar* +#+CAPTION[*lorem* ipsum]: dolar +1. baz From 6679db98a8c31d66126c607f88bdd525e65306bd Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Wed, 11 Oct 2023 17:53:32 -0400 Subject: [PATCH 02/10] Add comments. --- org_mode_samples/affiliated_keyword/optional_value.org | 5 +++++ src/compare/compare_field.rs | 5 +++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/org_mode_samples/affiliated_keyword/optional_value.org b/org_mode_samples/affiliated_keyword/optional_value.org index 99ebc24e..3b77d4c9 100644 --- a/org_mode_samples/affiliated_keyword/optional_value.org +++ b/org_mode_samples/affiliated_keyword/optional_value.org @@ -1,3 +1,8 @@ #+CAPTION[foo]: *bar* #+CAPTION[*lorem* ipsum]: dolar 1. baz + + +#+CAPTION[foo]: *bar* +#+CAPTION[*lorem* ipsum]: dolar +# Comments cannot have affiliated keywords so those become regular keywords. diff --git a/src/compare/compare_field.rs b/src/compare/compare_field.rs index 1fd82aa0..e56e316d 100644 --- a/src/compare/compare_field.rs +++ b/src/compare/compare_field.rs @@ -416,8 +416,9 @@ where /// /// Org-mode seems to store these as a 3-deep list: /// - Outer list with 1 element per #+caption keyword (or other parsed keyword). -/// - Middle list that seems to always have 1 element. -/// - Inner list of the objects from each #+caption keyword (or other parsed keyword). +/// - Middle list which has: +/// - first element is a list of objects representing the value after the colon. +/// - every additional element is a list of objects from inside the square brackets (the optional value). pub(crate) fn compare_property_list_of_list_of_list_of_ast_nodes< 'b, 's, From aeb2b6fe68cf26539816af439dfc4db7021f0341 Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Wed, 11 Oct 2023 18:29:07 -0400 Subject: [PATCH 03/10] Parse out the optional value objects. --- src/parser/affiliated_keyword.rs | 30 +++++++++++++++++++++++++++++- src/types/affiliated_keyword.rs | 2 +- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/src/parser/affiliated_keyword.rs b/src/parser/affiliated_keyword.rs index 0dc9cfa3..dc43820b 100644 --- a/src/parser/affiliated_keyword.rs +++ b/src/parser/affiliated_keyword.rs @@ -1,7 +1,18 @@ use std::collections::BTreeMap; +use nom::bytes::complete::tag; +use nom::bytes::complete::take_until; +use nom::character::complete::anychar; use nom::combinator::all_consuming; +use nom::combinator::eof; +use nom::combinator::map; +use nom::combinator::map_parser; +use nom::combinator::opt; +use nom::combinator::peek; +use nom::combinator::recognize; use nom::multi::many0; +use nom::multi::many_till; +use nom::sequence::tuple; use super::object_parser::standard_set_object; use crate::context::parser_with_context; @@ -45,6 +56,23 @@ pub(crate) fn parse_affiliated_keywords<'g, 's>( let initial_context = ContextElement::document_context(); let initial_context = Context::new(global_settings, List::new(&initial_context)); + let (_remaining, optional_objects) = opt(all_consuming(map( + tuple(( + take_until("["), + tag("["), + map_parser( + recognize(many_till(anychar, peek(tuple((tag("]"), eof))))), + all_consuming(many0(parser_with_context!(standard_set_object)( + &initial_context, + ))), + ), + tag("]"), + eof, + )), + |(_, _, objects, _, _)| objects, + )))(kw.key.into()) + .expect("Object parser should always succeed."); + // TODO: This should be omitting footnote references let (_remaining, objects) = all_consuming(many0(parser_with_context!( standard_set_object @@ -55,7 +83,7 @@ pub(crate) fn parse_affiliated_keywords<'g, 's>( }); match list_of_lists { AffiliatedKeywordValue::ListOfListsOfObjects(list_of_lists) => { - list_of_lists.push(objects); + list_of_lists.push((optional_objects, objects)); } _ => panic!("Invalid AffiliatedKeywordValue type."), } diff --git a/src/types/affiliated_keyword.rs b/src/types/affiliated_keyword.rs index f3dd94ea..5831a4d5 100644 --- a/src/types/affiliated_keyword.rs +++ b/src/types/affiliated_keyword.rs @@ -6,7 +6,7 @@ use super::Object; pub enum AffiliatedKeywordValue<'s> { SingleString(&'s str), ListOfStrings(Vec<&'s str>), - ListOfListsOfObjects(Vec>>), + ListOfListsOfObjects(Vec<(Option>>, Vec>)>), } #[derive(Debug)] From b1a0fa4acf1cdae401cf4c3fd7c4f515cadd1a78 Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Wed, 11 Oct 2023 18:44:21 -0400 Subject: [PATCH 04/10] Compare optional value. --- src/compare/compare_field.rs | 50 ++++++++++++++++++++++++++---------- 1 file changed, 37 insertions(+), 13 deletions(-) diff --git a/src/compare/compare_field.rs b/src/compare/compare_field.rs index e56e316d..27a79228 100644 --- a/src/compare/compare_field.rs +++ b/src/compare/compare_field.rs @@ -424,7 +424,7 @@ pub(crate) fn compare_property_list_of_list_of_list_of_ast_nodes< 's, 'x, R, - RG: Fn(R) -> Option<&'b Vec>>>, + RG: Fn(R) -> Option<&'b Vec<(Option>>, Vec>)>>, >( source: &'s str, emacs: &'b Token<'s>, @@ -461,10 +461,41 @@ pub(crate) fn compare_property_list_of_list_of_list_of_ast_nodes< }; // Iterate the outer lists - for (value, rust_value) in value.iter().zip(rust_value.iter()) { - // Assert the middle list is a length of 1 because I've never seen it any other way. - let value = value.as_list()?; - if value.len() != 1 { + for (value, (rust_optional, rust_value)) in value.iter().zip(rust_value.iter()) { + let mut middle_value = value.as_list()?.iter(); + // First element of middle list is the mandatory value (the value past the colon). + let mandatory_value = middle_value.next(); + let mandatory_value = match mandatory_value { + Some(mandatory_value) => mandatory_value, + None => { + let this_status = DiffStatus::Bad; + let message = Some(format!( + "{} mismatch (emacs != rust) {:?} != {:?}", + emacs_field, value, rust_value + )); + return Ok(ComparePropertiesResult::SelfChange(this_status, message)); + } + }; + let mut child_status: Vec> = Vec::with_capacity(rust_value.len()); + + // Compare optional value + if let Some(rust_optional) = rust_optional { + if rust_optional.len() != middle_value.len() { + let this_status = DiffStatus::Bad; + let message = Some(format!( + "{} mismatch (emacs != rust) {:?} != {:?}", + emacs_field, value, rust_value + )); + return Ok(ComparePropertiesResult::SelfChange(this_status, message)); + } + for (e, r) in middle_value.zip(rust_optional) { + child_status.push(compare_ast_node(source, e, r.into())?); + } + } + + // Compare mandatory value + let mandatory_value = mandatory_value.as_list()?; + if rust_value.len() != mandatory_value.len() { let this_status = DiffStatus::Bad; let message = Some(format!( "{} mismatch (emacs != rust) {:?} != {:?}", @@ -472,14 +503,7 @@ pub(crate) fn compare_property_list_of_list_of_list_of_ast_nodes< )); return Ok(ComparePropertiesResult::SelfChange(this_status, message)); } - // Drill past the middle list to the inner list. - let value = value - .first() - .expect("The above if-statement asserts this exists."); - let value = value.as_list()?; - // Compare inner lists - let mut child_status: Vec> = Vec::with_capacity(rust_value.len()); - for (e, r) in value.iter().zip(rust_value) { + for (e, r) in mandatory_value.iter().zip(rust_value) { child_status.push(compare_ast_node(source, e, r.into())?); } let diff_scope = artificial_owned_diff_scope(emacs_field, child_status)?; From 51429e31553f0cfc7db55db363b5d1c3a1086f44 Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Wed, 11 Oct 2023 18:50:22 -0400 Subject: [PATCH 05/10] Handle optval when translating names. --- src/parser/affiliated_keyword.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/parser/affiliated_keyword.rs b/src/parser/affiliated_keyword.rs index dc43820b..88cdabdb 100644 --- a/src/parser/affiliated_keyword.rs +++ b/src/parser/affiliated_keyword.rs @@ -103,12 +103,16 @@ pub(crate) fn parse_affiliated_keywords<'g, 's>( } fn translate_name<'g, 's>(global_settings: &'g GlobalSettings<'g, 's>, name: &'s str) -> String { + let name_until_optval = name + .split_once("[") + .map(|(before, _after)| before) + .unwrap_or(name); for (src, dst) in global_settings.element_keyword_translation_alist { - if name.eq_ignore_ascii_case(src) { + if name_until_optval.eq_ignore_ascii_case(src) { return dst.to_lowercase(); } } - name.to_lowercase() + name_until_optval.to_lowercase() } fn is_single_string_keyword<'g, 's>( From efc6bd11d940a5285387fa984adbbadda773b0a8 Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Wed, 11 Oct 2023 18:59:13 -0400 Subject: [PATCH 06/10] Do not exit on first loop. --- src/compare/compare_field.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/compare/compare_field.rs b/src/compare/compare_field.rs index 27a79228..ed152245 100644 --- a/src/compare/compare_field.rs +++ b/src/compare/compare_field.rs @@ -460,6 +460,8 @@ pub(crate) fn compare_property_list_of_list_of_list_of_ast_nodes< (Some(value), Some(rust_value)) => (value, rust_value), }; + let mut child_status: Vec> = Vec::with_capacity(rust_value.len()); + // Iterate the outer lists for (value, (rust_optional, rust_value)) in value.iter().zip(rust_value.iter()) { let mut middle_value = value.as_list()?.iter(); @@ -476,7 +478,6 @@ pub(crate) fn compare_property_list_of_list_of_list_of_ast_nodes< return Ok(ComparePropertiesResult::SelfChange(this_status, message)); } }; - let mut child_status: Vec> = Vec::with_capacity(rust_value.len()); // Compare optional value if let Some(rust_optional) = rust_optional { @@ -489,6 +490,7 @@ pub(crate) fn compare_property_list_of_list_of_list_of_ast_nodes< return Ok(ComparePropertiesResult::SelfChange(this_status, message)); } for (e, r) in middle_value.zip(rust_optional) { + eprintln!("Comparing optval! {:?} {:?}", e, r); child_status.push(compare_ast_node(source, e, r.into())?); } } @@ -504,12 +506,16 @@ pub(crate) fn compare_property_list_of_list_of_list_of_ast_nodes< return Ok(ComparePropertiesResult::SelfChange(this_status, message)); } for (e, r) in mandatory_value.iter().zip(rust_value) { + eprintln!("Comparing mandatory! {:?} {:?}", e, r); child_status.push(compare_ast_node(source, e, r.into())?); } - let diff_scope = artificial_owned_diff_scope(emacs_field, child_status)?; - return Ok(ComparePropertiesResult::DiffEntry(diff_scope)); } - Ok(ComparePropertiesResult::NoChange) + if child_status.is_empty() { + Ok(ComparePropertiesResult::NoChange) + } else { + let diff_scope = artificial_owned_diff_scope(emacs_field, child_status)?; + Ok(ComparePropertiesResult::DiffEntry(diff_scope)) + } } pub(crate) fn compare_property_number_lines< From 4a44d8846146521acf8aa733eaf4ab57b7d10237 Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Wed, 11 Oct 2023 19:05:39 -0400 Subject: [PATCH 07/10] Better error messages. --- src/compare/compare_field.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/compare/compare_field.rs b/src/compare/compare_field.rs index ed152245..797f1ef2 100644 --- a/src/compare/compare_field.rs +++ b/src/compare/compare_field.rs @@ -484,8 +484,11 @@ pub(crate) fn compare_property_list_of_list_of_list_of_ast_nodes< if rust_optional.len() != middle_value.len() { let this_status = DiffStatus::Bad; let message = Some(format!( - "{} mismatch (emacs != rust) {:?} != {:?}", - emacs_field, value, rust_value + "{} optional value length mismatch (emacs != rust) {} != {} | {:?}", + emacs_field, + middle_value.len(), + rust_optional.len(), + rust_optional )); return Ok(ComparePropertiesResult::SelfChange(this_status, message)); } @@ -500,8 +503,11 @@ pub(crate) fn compare_property_list_of_list_of_list_of_ast_nodes< if rust_value.len() != mandatory_value.len() { let this_status = DiffStatus::Bad; let message = Some(format!( - "{} mismatch (emacs != rust) {:?} != {:?}", - emacs_field, value, rust_value + "{} mandatory value length mismatch (emacs != rust) {} != {} | {:?}", + emacs_field, + mandatory_value.len(), + rust_value.len(), + rust_value )); return Ok(ComparePropertiesResult::SelfChange(this_status, message)); } From c4edcb8c244a7d78dacfb269d1bb43d2cc5ca5ad Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Wed, 11 Oct 2023 19:07:47 -0400 Subject: [PATCH 08/10] Fix parsing the optional value. --- src/parser/affiliated_keyword.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/parser/affiliated_keyword.rs b/src/parser/affiliated_keyword.rs index 88cdabdb..498033f8 100644 --- a/src/parser/affiliated_keyword.rs +++ b/src/parser/affiliated_keyword.rs @@ -15,6 +15,7 @@ use nom::multi::many_till; use nom::sequence::tuple; use super::object_parser::standard_set_object; +use super::util::confine_context; use crate::context::parser_with_context; use crate::context::Context; use crate::context::ContextElement; @@ -62,9 +63,11 @@ pub(crate) fn parse_affiliated_keywords<'g, 's>( tag("["), map_parser( recognize(many_till(anychar, peek(tuple((tag("]"), eof))))), - all_consuming(many0(parser_with_context!(standard_set_object)( - &initial_context, - ))), + confine_context(|i| { + all_consuming(many0(parser_with_context!(standard_set_object)( + &initial_context, + )))(i) + }), ), tag("]"), eof, From dff7550038fd133a92f0df5e2de850c6b220edba Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Wed, 11 Oct 2023 19:09:51 -0400 Subject: [PATCH 09/10] Remove prints. --- src/compare/compare_field.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/compare/compare_field.rs b/src/compare/compare_field.rs index 797f1ef2..edf86b28 100644 --- a/src/compare/compare_field.rs +++ b/src/compare/compare_field.rs @@ -493,7 +493,6 @@ pub(crate) fn compare_property_list_of_list_of_list_of_ast_nodes< return Ok(ComparePropertiesResult::SelfChange(this_status, message)); } for (e, r) in middle_value.zip(rust_optional) { - eprintln!("Comparing optval! {:?} {:?}", e, r); child_status.push(compare_ast_node(source, e, r.into())?); } } @@ -512,7 +511,6 @@ pub(crate) fn compare_property_list_of_list_of_list_of_ast_nodes< return Ok(ComparePropertiesResult::SelfChange(this_status, message)); } for (e, r) in mandatory_value.iter().zip(rust_value) { - eprintln!("Comparing mandatory! {:?} {:?}", e, r); child_status.push(compare_ast_node(source, e, r.into())?); } } From 8bc942a26f796d8c775fb2e839e9253881c7b870 Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Wed, 11 Oct 2023 19:13:45 -0400 Subject: [PATCH 10/10] Create artificial scopes for the optional value and mandatory value. --- src/compare/compare_field.rs | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/compare/compare_field.rs b/src/compare/compare_field.rs index edf86b28..01896aad 100644 --- a/src/compare/compare_field.rs +++ b/src/compare/compare_field.rs @@ -460,7 +460,7 @@ pub(crate) fn compare_property_list_of_list_of_list_of_ast_nodes< (Some(value), Some(rust_value)) => (value, rust_value), }; - let mut child_status: Vec> = Vec::with_capacity(rust_value.len()); + let mut full_status: Vec> = Vec::with_capacity(rust_value.len()); // Iterate the outer lists for (value, (rust_optional, rust_value)) in value.iter().zip(rust_value.iter()) { @@ -481,6 +481,7 @@ pub(crate) fn compare_property_list_of_list_of_list_of_ast_nodes< // Compare optional value if let Some(rust_optional) = rust_optional { + let mut child_status: Vec> = Vec::with_capacity(rust_value.len()); if rust_optional.len() != middle_value.len() { let this_status = DiffStatus::Bad; let message = Some(format!( @@ -495,9 +496,14 @@ pub(crate) fn compare_property_list_of_list_of_list_of_ast_nodes< for (e, r) in middle_value.zip(rust_optional) { child_status.push(compare_ast_node(source, e, r.into())?); } + if !child_status.is_empty() { + let diff_scope = artificial_diff_scope("optional value", child_status)?; + full_status.push(diff_scope); + } } // Compare mandatory value + let mut child_status: Vec> = Vec::with_capacity(rust_value.len()); let mandatory_value = mandatory_value.as_list()?; if rust_value.len() != mandatory_value.len() { let this_status = DiffStatus::Bad; @@ -513,11 +519,15 @@ pub(crate) fn compare_property_list_of_list_of_list_of_ast_nodes< for (e, r) in mandatory_value.iter().zip(rust_value) { child_status.push(compare_ast_node(source, e, r.into())?); } + if !child_status.is_empty() { + let diff_scope = artificial_diff_scope("mandatory value", child_status)?; + full_status.push(diff_scope); + } } - if child_status.is_empty() { + if full_status.is_empty() { Ok(ComparePropertiesResult::NoChange) } else { - let diff_scope = artificial_owned_diff_scope(emacs_field, child_status)?; + let diff_scope = artificial_owned_diff_scope(emacs_field, full_status)?; Ok(ComparePropertiesResult::DiffEntry(diff_scope)) } }