From 439bbfe88e971bfa5f07a1faec8ff4a48bbeeb86 Mon Sep 17 00:00:00 2001 From: Daniel Silverstone Date: Wed, 27 Dec 2023 17:37:27 +0000 Subject: bindings: Remember filename when loading bindings Signed-off-by: Daniel Silverstone --- src/bindings.rs | 148 +++++++++++++++++++++++++++++++++++++++++--------- tests/bindings-ubm.rs | 9 ++- 2 files changed, 130 insertions(+), 27 deletions(-) diff --git a/src/bindings.rs b/src/bindings.rs index b937edb..c342432 100644 --- a/src/bindings.rs +++ b/src/bindings.rs @@ -167,6 +167,7 @@ pub struct Binding { impls: HashMap>, types: HashMap, doc: Option, + filename: Arc, } impl Binding { @@ -177,6 +178,7 @@ impl Binding { case_sensitive: bool, mut types: HashMap, doc: Option, + filename: Arc, ) -> Result { let regex = RegexBuilder::new(&format!("^{pattern}$")) .case_insensitive(!case_sensitive) @@ -196,6 +198,7 @@ impl Binding { impls: HashMap::new(), types, doc, + filename, }) } @@ -307,6 +310,10 @@ impl Binding { Some(m) } + + fn filename(&self) -> &Path { + &self.filename + } } impl PartialEq for Binding { @@ -325,10 +332,25 @@ mod test_binding { use crate::ScenarioStep; use crate::StepKind; use std::collections::HashMap; + use std::path::Path; + use std::path::PathBuf; + use std::sync::Arc; + + fn path() -> Arc { + PathBuf::new().into() + } #[test] fn creates_new() { - let b = Binding::new(StepKind::Given, "I am Tomjon", false, HashMap::new(), None).unwrap(); + let b = Binding::new( + StepKind::Given, + "I am Tomjon", + false, + HashMap::new(), + None, + path(), + ) + .unwrap(); assert_eq!(b.kind(), StepKind::Given); assert!(b.regex().is_match("I am Tomjon")); assert!(!b.regex().is_match("I am Tomjon of Lancre")); @@ -337,20 +359,45 @@ mod test_binding { #[test] fn equal() { - let a = Binding::new(StepKind::Given, "I am Tomjon", false, HashMap::new(), None).unwrap(); - let b = Binding::new(StepKind::Given, "I am Tomjon", false, HashMap::new(), None).unwrap(); + let a = Binding::new( + StepKind::Given, + "I am Tomjon", + false, + HashMap::new(), + None, + path(), + ) + .unwrap(); + let b = Binding::new( + StepKind::Given, + "I am Tomjon", + false, + HashMap::new(), + None, + path(), + ) + .unwrap(); assert_eq!(a, b); } #[test] fn not_equal() { - let a = Binding::new(StepKind::Given, "I am Tomjon", false, HashMap::new(), None).unwrap(); + let a = Binding::new( + StepKind::Given, + "I am Tomjon", + false, + HashMap::new(), + None, + path(), + ) + .unwrap(); let b = Binding::new( StepKind::Given, "I am Tomjon of Lancre", false, HashMap::new(), None, + path(), ) .unwrap(); assert_ne!(a, b); @@ -359,21 +406,21 @@ mod test_binding { #[test] fn does_not_match_with_wrong_kind() { let step = ScenarioStep::new(StepKind::Given, "given", "yo", Location::Unknown); - let b = Binding::new(StepKind::When, "yo", false, HashMap::new(), None).unwrap(); + let b = Binding::new(StepKind::When, "yo", false, HashMap::new(), None, path()).unwrap(); assert!(b.match_with_step("", &step).is_none()); } #[test] fn does_not_match_with_wrong_text() { let step = ScenarioStep::new(StepKind::Given, "given", "foo", Location::Unknown); - let b = Binding::new(StepKind::Given, "bar", false, HashMap::new(), None).unwrap(); + let b = Binding::new(StepKind::Given, "bar", false, HashMap::new(), None, path()).unwrap(); assert!(b.match_with_step("", &step).is_none()); } #[test] fn match_with_fixed_pattern() { let step = ScenarioStep::new(StepKind::Given, "given", "foo", Location::Unknown); - let b = Binding::new(StepKind::Given, "foo", false, HashMap::new(), None).unwrap(); + let b = Binding::new(StepKind::Given, "foo", false, HashMap::new(), None, path()).unwrap(); let m = b.match_with_step("", &step).unwrap(); assert_eq!(m.kind(), StepKind::Given); let mut parts = m.parts(); @@ -396,6 +443,7 @@ mod test_binding { false, HashMap::new(), None, + path(), ) .unwrap(); let m = b.match_with_step("", &step).unwrap(); @@ -413,9 +461,25 @@ mod test_binding { #[test] fn case_sensitive_mismatch() { let step = ScenarioStep::new(StepKind::Given, "given", "I am Tomjon", Location::Unknown); - let b = Binding::new(StepKind::Given, r"i am tomjon", false, HashMap::new(), None).unwrap(); + let b = Binding::new( + StepKind::Given, + r"i am tomjon", + false, + HashMap::new(), + None, + path(), + ) + .unwrap(); assert!(b.match_with_step("", &step).is_some()); - let b = Binding::new(StepKind::Given, r"i am tomjon", true, HashMap::new(), None).unwrap(); + let b = Binding::new( + StepKind::Given, + r"i am tomjon", + true, + HashMap::new(), + None, + path(), + ) + .unwrap(); assert!(b.match_with_step("", &step).is_none()); } } @@ -493,11 +557,11 @@ impl Bindings { } /// Add bindings from a YAML string - pub fn add_from_yaml(&mut self, yaml: &str) -> Result<(), SubplotError> { + pub fn add_from_yaml(&mut self, yaml: &str, filename: Arc) -> Result<(), SubplotError> { let bindings: Vec = serde_yaml::from_str(yaml).map_err(SubplotError::Metadata)?; for wrapper in bindings { - self.add(from_hashmap(&wrapper.binding)?); + self.add(from_hashmap(&wrapper.binding, Arc::clone(&filename))?); } Ok(()) } @@ -539,12 +603,12 @@ impl Bindings { where P: AsRef + Debug, { - let yaml = resource::read_as_string(filename.as_ref(), template) - .map_err(|e| SubplotError::BindingsFileNotFound(filename.as_ref().into(), e))?; + let filename = filename.as_ref(); + let yaml = resource::read_as_string(filename, template) + .map_err(|e| SubplotError::BindingsFileNotFound(filename.into(), e))?; trace!("Loaded file content"); - self.add_from_yaml(&yaml).map_err(|e| { - SubplotError::BindingFileParseError(filename.as_ref().to_owned(), Box::new(e)) - })?; + self.add_from_yaml(&yaml, filename.to_owned().into()) + .map_err(|e| SubplotError::BindingFileParseError(filename.to_owned(), Box::new(e)))?; Ok(()) } @@ -558,7 +622,7 @@ impl Bindings { } } -fn from_hashmap(parsed: &ParsedBinding) -> Result { +fn from_hashmap(parsed: &ParsedBinding, filename: Arc) -> Result { let given: i32 = parsed.given.is_some().into(); let when: i32 = parsed.when.is_some().into(); let then: i32 = parsed.then.is_some().into(); @@ -601,6 +665,7 @@ fn from_hashmap(parsed: &ParsedBinding) -> Result { parsed.case_sensitive, types, parsed.doc.clone(), + filename, )?; trace!("Binding parsed OK"); for (template, pimpl) in &parsed.impls { @@ -622,6 +687,13 @@ mod test_bindings { use crate::SubplotError; use std::collections::HashMap; + use std::path::Path; + use std::path::PathBuf; + use std::sync::Arc; + + fn path() -> Arc { + PathBuf::new().into() + } #[test] fn has_no_bindings_initially() { @@ -637,6 +709,7 @@ mod test_bindings { false, HashMap::new(), None, + path(), ) .unwrap(); let mut bindings = Bindings::new(); @@ -672,7 +745,7 @@ mod test_bindings { total: word "; let mut bindings = Bindings::new(); - bindings.add_from_yaml(yaml).unwrap(); + bindings.add_from_yaml(yaml, path()).unwrap(); println!("test: {bindings:?}"); assert!(bindings.has(StepKind::Given, "I am Tomjon")); assert!(bindings.has(StepKind::When, "I declare myself king")); @@ -692,7 +765,7 @@ mod test_bindings { python: FUNCTION: set_name "; - match Bindings::new().add_from_yaml(yaml) { + match Bindings::new().add_from_yaml(yaml, path()) { Ok(_) => unreachable!(), Err(SubplotError::BindingHasManyKeywords(_)) => (), Err(e) => panic!("Incorrect error: {}", e), @@ -709,7 +782,7 @@ mod test_bindings { types: age: number "; - match Bindings::new().add_from_yaml(yaml) { + match Bindings::new().add_from_yaml(yaml, path()) { Ok(_) => unreachable!(), Err(SubplotError::SimplePatternKindMismatch(_)) => (), Err(e) => panic!("Incorrect error: {}", e), @@ -719,8 +792,15 @@ mod test_bindings { #[test] fn does_not_find_match_for_unmatching_kind() { let step = ScenarioStep::new(StepKind::Given, "given", "I am Tomjon", Location::Unknown); - let binding = - Binding::new(StepKind::When, r"I am Tomjon", false, HashMap::new(), None).unwrap(); + let binding = Binding::new( + StepKind::When, + r"I am Tomjon", + false, + HashMap::new(), + None, + path(), + ) + .unwrap(); let mut bindings = Bindings::new(); bindings.add(binding); assert!(matches!( @@ -738,6 +818,7 @@ mod test_bindings { false, HashMap::new(), None, + path(), ) .unwrap(); let mut bindings = Bindings::new(); @@ -753,7 +834,15 @@ mod test_bindings { let step = ScenarioStep::new(StepKind::Given, "given", "I am Tomjon", Location::Unknown); let mut bindings = Bindings::default(); bindings.add( - Binding::new(StepKind::Given, r"I am Tomjon", false, HashMap::new(), None).unwrap(), + Binding::new( + StepKind::Given, + r"I am Tomjon", + false, + HashMap::new(), + None, + path(), + ) + .unwrap(), ); bindings.add( Binding::new( @@ -763,6 +852,7 @@ mod test_bindings { false, HashMap::new(), None, + path(), ) .unwrap(), ); @@ -775,8 +865,15 @@ mod test_bindings { #[test] fn finds_match_for_fixed_string_pattern() { let step = ScenarioStep::new(StepKind::Given, "given", "I am Tomjon", Location::Unknown); - let binding = - Binding::new(StepKind::Given, r"I am Tomjon", false, HashMap::new(), None).unwrap(); + let binding = Binding::new( + StepKind::Given, + r"I am Tomjon", + false, + HashMap::new(), + None, + path(), + ) + .unwrap(); let mut bindings = Bindings::new(); bindings.add(binding); let m = bindings.find("", &step).unwrap(); @@ -799,6 +896,7 @@ mod test_bindings { false, HashMap::new(), None, + path(), ) .unwrap(); let mut bindings = Bindings::new(); diff --git a/tests/bindings-ubm.rs b/tests/bindings-ubm.rs index 4c4aaa0..e4232d1 100644 --- a/tests/bindings-ubm.rs +++ b/tests/bindings-ubm.rs @@ -4,12 +4,17 @@ // there are a large number of them. use regex::RegexBuilder; -use std::collections::HashMap; +use std::path::{Path, PathBuf}; use std::time::SystemTime; +use std::{collections::HashMap, sync::Arc}; use subplot::{html::Location, Binding, Bindings, ScenarioStep, StepKind}; const N: i32 = 1000; +fn path() -> Arc { + PathBuf::new().into() +} + #[test] fn bindings_microbenchmark() { let time = SystemTime::now(); @@ -34,7 +39,7 @@ fn bindings_microbenchmark() { let mut toadd = vec![]; for t in texts.iter() { - toadd.push(Binding::new(StepKind::Given, t, false, HashMap::new(), None).unwrap()); + toadd.push(Binding::new(StepKind::Given, t, false, HashMap::new(), None, path()).unwrap()); } let created = time.elapsed().unwrap(); -- cgit v1.2.1 From da1774ea591a7c0bc241e4d0c16649a1cf485c13 Mon Sep 17 00:00:00 2001 From: Daniel Silverstone Date: Wed, 27 Dec 2023 18:19:12 +0000 Subject: bindings: Add warnings for missing capture names or missing capture types Signed-off-by: Daniel Silverstone --- src/bin/subplot.rs | 1 + src/bindings.rs | 53 ++++++++++++++++++++++++++++++++++++++++++----------- src/doc.rs | 5 +++++ src/error.rs | 8 ++++++++ 4 files changed, 56 insertions(+), 11 deletions(-) diff --git a/src/bin/subplot.rs b/src/bin/subplot.rs index fd9500d..018ae5b 100644 --- a/src/bin/subplot.rs +++ b/src/bin/subplot.rs @@ -493,6 +493,7 @@ fn load_linted_doc( let template = template.to_string(); trace!("Template: {:#?}", template); let mut warnings = Warnings::default(); + doc.check_bindings(&mut warnings)?; doc.check_named_code_blocks_have_appropriate_class(&mut warnings)?; doc.check_named_files_exist(&template, &mut warnings)?; doc.check_matched_steps_have_impl(&template, &mut warnings); diff --git a/src/bindings.rs b/src/bindings.rs index c342432..825b895 100644 --- a/src/bindings.rs +++ b/src/bindings.rs @@ -3,6 +3,7 @@ use super::MatchedSteps; use super::PartialStep; use super::ScenarioStep; use super::StepKind; +use crate::Warning; use crate::{resource, SubplotError}; use serde::{Deserialize, Serialize}; @@ -176,7 +177,7 @@ impl Binding { kind: StepKind, pattern: &str, case_sensitive: bool, - mut types: HashMap, + types: HashMap, doc: Option, filename: Arc, ) -> Result { @@ -184,12 +185,6 @@ impl Binding { .case_insensitive(!case_sensitive) .build() .map_err(|err| SubplotError::Regex(pattern.to_string(), err))?; - // For every named capture, ensure we have a known type for it. - // If the type is missing from the map, we default to `text` which is - // the .* pattern - for capture in regex.capture_names().flatten() { - types.entry(capture.into()).or_insert(CaptureType::Text); - } Ok(Binding { kind, @@ -286,15 +281,14 @@ impl Binding { let cap = cap.as_str(); // These unwraps are safe because we ensured the map is complete // in the constructor, and that all the types are known. - let ty = self.types.get(name).unwrap(); - let rx = &KIND_PATTERNS.get(ty).unwrap(); + let kind = self.types.get(name).copied().unwrap_or(CaptureType::Text); + let rx = KIND_PATTERNS.get(&kind).unwrap(); if !rx.is_match(cap) { // This capture doesn't match the kind so it's not // valid for this binding. return None; } - let kind = self.types.get(name).unwrap_or(&CaptureType::Text); - PartialStep::text(name, cap, *kind) + PartialStep::text(name, cap, kind) } }; @@ -314,6 +308,35 @@ impl Binding { fn filename(&self) -> &Path { &self.filename } + + fn check(&self, warnings: &mut crate::Warnings) -> Result<(), SubplotError> { + fn nth(i: usize) -> &'static str { + match i % 10 { + 1 => "st", + 2 => "nd", + 3 => "rd", + _ => "th", + } + } + for (nr, capture) in self.regex.capture_names().enumerate().skip(1) { + if let Some(name) = capture { + if !self.types.contains_key(name) { + warnings.push(Warning::MissingCaptureType( + self.filename().to_owned(), + format!("{}: {}", self.kind(), self.pattern()), + name.to_string(), + )); + } + } else { + warnings.push(Warning::MissingCaptureName( + self.filename().to_owned(), + format!("{}: {}", self.kind(), self.pattern()), + format!("{nr}{}", nth(nr)), + )); + } + } + Ok(()) + } } impl PartialEq for Binding { @@ -620,6 +643,14 @@ impl Bindings { .filter(|b| b.kind() == kind && b.pattern() == pattern); m.count() == 1 } + + /// Check these bindings for any warnings which users might need to know about + pub fn check(&self, warnings: &mut crate::Warnings) -> Result<(), SubplotError> { + for binding in self.bindings() { + binding.check(warnings)?; + } + Ok(()) + } } fn from_hashmap(parsed: &ParsedBinding, filename: Arc) -> Result { diff --git a/src/doc.rs b/src/doc.rs index 1e48e40..ff52b79 100644 --- a/src/doc.rs +++ b/src/doc.rs @@ -474,6 +474,11 @@ impl Document { set } + /// Check bindings for warnings + pub fn check_bindings(&self, warnings: &mut Warnings) -> Result<(), SubplotError> { + self.meta.bindings().check(warnings) + } + /// Check labelled code blocks have some appropriate class pub fn check_named_code_blocks_have_appropriate_class( &self, diff --git a/src/error.rs b/src/error.rs index 7c63ca4..b89e94b 100644 --- a/src/error.rs +++ b/src/error.rs @@ -394,6 +394,14 @@ pub enum Warning { /// A code block has an identifier but is not marked as a file or example #[error("Code block has identifier but lacks file or example class. Is this a mistake? #{0} at {1}")] MissingAppropriateClassOnNamedCodeBlock(String, String), + + /// A capture in a binding is missing a name + #[error("{0}: {1} - missing a name for the {2} capture")] + MissingCaptureName(PathBuf, String, String), + + /// A capture in a binding is missing a type + #[error("{0}: {1} - missing a type for the capture called {2}")] + MissingCaptureType(PathBuf, String, String), } /// A list of warnings. -- cgit v1.2.1 From ffd655aed8807a75d5e89ae041823d7bb5cfed13 Mon Sep 17 00:00:00 2001 From: Daniel Silverstone Date: Wed, 27 Dec 2023 18:19:38 +0000 Subject: bindings: Add types for all relevant bindings which were missing them Signed-off-by: Daniel Silverstone --- examples/muck/muck.yaml | 18 ++++++++++++++++++ share/common/lib/files.yaml | 2 ++ subplot.yaml | 16 ++++++++++++++++ 3 files changed, 36 insertions(+) diff --git a/examples/muck/muck.yaml b/examples/muck/muck.yaml index b22e088..967ab56 100644 --- a/examples/muck/muck.yaml +++ b/examples/muck/muck.yaml @@ -18,24 +18,34 @@ python: function: fixme regex: true + types: + json: text - when: "I do PUT /res with Muck-Id: \\{(?P\\S+)\\}, Muck-Revision: \\{(?P\\S+)\\}, and body (?P\\{.*\\})" impl: python: function: fixme regex: true + types: + id: word + rev: word + json: text - when: "I do GET /res with Muck-Id: \\{(?P\\S+)\\}" impl: python: function: fixme regex: true + types: + id: word - when: "I do DELETE /res with Muck-Id: \\{(?P\\S+)\\}" impl: python: function: fixme regex: true + types: + id: word - when: "I restart Muck" impl: @@ -58,15 +68,23 @@ python: function: fixme regex: true + types: + header: word + name: word - then: "body matches (?P\\{.*\\})" impl: python: function: fixme regex: true + types: + json: text - then: "revisions \\{(?P\\S+)\\} and \\{(?P\\S+)\\} are different" impl: python: function: fixme regex: true + types: + rev1: word + rev2: word diff --git a/share/common/lib/files.yaml b/share/common/lib/files.yaml index 8f3f932..689a5f3 100644 --- a/share/common/lib/files.yaml +++ b/share/common/lib/files.yaml @@ -242,6 +242,8 @@ python: function: files_only_these_exist regex: true + types: + filenames: text doc: | Check that the test directory only contains specific files. diff --git a/subplot.yaml b/subplot.yaml index 5ad35aa..1c384eb 100644 --- a/subplot.yaml +++ b/subplot.yaml @@ -28,6 +28,9 @@ rust: function: step_was_run regex: true + types: + keyword: text + name: text - then: step "(?Pgiven|when|then) (?P.+)" was run, and then step "(?Pgiven|when|then) (?P.+)" impl: @@ -36,6 +39,11 @@ rust: function: step_was_run_and_then regex: true + types: + keyword1: text + keyword2: text + name1: text + name2: text - then: cleanup for "(?Pgiven|when|then) (?P.+)" was run, and then for "(?Pgiven|when|then) (?P.+)" impl: @@ -44,6 +52,11 @@ rust: function: cleanup_was_run regex: true + types: + keyword1: text + keyword2: text + name1: text + name2: text - then: cleanup for "(?Pgiven|when|then) (?P.+)" was not run impl: @@ -52,6 +65,9 @@ rust: function: cleanup_was_not_run regex: true + types: + keyword: text + name: text - then: JSON output matches {filename} impl: -- cgit v1.2.1 From 2502d8789c4f8d92236dbc0b732d3fba912b497e Mon Sep 17 00:00:00 2001 From: Daniel Silverstone Date: Wed, 27 Dec 2023 18:19:53 +0000 Subject: reference: Use --merciful because we might have added new warnings Signed-off-by: Daniel Silverstone --- reference.md | 2 +- reference.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/reference.md b/reference.md index 1776e0b..9999636 100644 --- a/reference.md +++ b/reference.md @@ -12,7 +12,7 @@ set of subplot documents. given an installed subplot given a clone of https://gitlab.com/subplot/subplot.git in src at 5168420454b92205c13224a6801d3341d7f0c3d3 when I docgen subplot.subplot to test.html, in src -when I run, in src, subplot docgen subplot.subplot -o subplot.html -t python +when I run, in src, subplot docgen subplot.subplot --merciful -o subplot.html -t python then file src/test.html exists ~~~ diff --git a/reference.py b/reference.py index fa4b274..803716a 100644 --- a/reference.py +++ b/reference.py @@ -15,7 +15,7 @@ def docgen(ctx, filename=None, output=None, dirname=None): runcmd_run( ctx, - ["subplot", "docgen", filename, "--output", output, "-t", "python"], + ["subplot", "docgen", filename, "--merciful", "--output", output, "-t", "python"], cwd=dirname, ) runcmd_exit_code_is_zero(ctx) -- cgit v1.2.1