diff --git a/.yamllint b/.yamllint new file mode 100644 index 00000000..626837bc --- /dev/null +++ b/.yamllint @@ -0,0 +1,3 @@ +extends: default +ignore: | + tests/data/jinja_for*.yml diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index 8c0fb969..381d6bca 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -161,9 +161,9 @@ level keys. future evolution of the schema while maintaining backward compatibility. This version string should be parsed and validated using the `semver` crate.[^4] -- `vars`: A mapping of global key-value string pairs. These variables are - available for substitution in rule commands and target definitions and are - exposed to the Jinja templating context. +- `vars`: A mapping of global key-value pairs. Keys must be strings. Values may + be strings, numbers, booleans, or sequences. These variables seed the Jinja + templating context and drive control flow within the manifest. - `macros`: An optional list of Jinja macro definitions. Each item provides a `signature` string using standard Jinja syntax and a `body` declared with the @@ -414,7 +414,7 @@ pub struct NetsukeManifest { pub netsuke_version: Version, #[serde(default)] - pub vars: HashMap, + pub vars: HashMap, #[serde(default)] pub rules: Vec, @@ -466,7 +466,7 @@ pub struct Target { pub order_only_deps: StringOrList, #[serde(default)] - pub vars: HashMap, + pub vars: HashMap, /// Run this target when requested even if a file with the same name exists. #[serde(default)] @@ -585,15 +585,14 @@ Unknown fields are rejected to surface user errors early. `StringOrList` provides a default `Empty` variant, so optional lists are trivial to represent. The manifest version is parsed using the `semver` crate to validate that it follows semantic versioning rules. Global and target variable maps now share -the `HashMap` type for consistency. This keeps YAML manifests -concise while ensuring forward compatibility. Targets also accept optional -`phony` and `always` booleans. They default to `false`, making it explicit when -an action should run regardless of file timestamps. Targets listed in the -`actions` section are deserialised using a custom helper so they are always -treated as `phony` tasks. This ensures preparation actions never generate build -artefacts. Convenience functions in `src/manifest.rs` load a manifest from a -string or a file path, returning `anyhow::Result` for straightforward error -handling. +the `HashMap` type so booleans and sequences are +preserved for Jinja control flow. Targets also accept optional `phony` and +`always` booleans. They default to `false`, making it explicit when an action +should run regardless of file timestamps. Targets listed in the `actions` +section are deserialised using a custom helper so they are always treated as +`phony` tasks. This ensures preparation actions never generate build artefacts. +Convenience functions in `src/manifest.rs` load a manifest from a string or a +file path, returning `anyhow::Result` for straightforward error handling. ### 3.5 Testing @@ -666,6 +665,13 @@ lenient undefined behaviour. The resulting YAML is parsed to obtain the global variables, which are then injected into the environment before a second, strict render pass produces the final manifest for deserialisation. +The parser copies `vars` values into the environment using +`Value::from_serializable`. This preserves native YAML types so Jinja's +`{% if %}` and `{% for %}` constructs can branch on booleans or iterate over +sequences. Keys must be strings; any non-string key causes manifest parsing to +fail. Attempting to iterate over a non-sequence results in a render error +surfaced during manifest loading. + ### 4.3 User-Defined Macros Netsuke allows users to declare reusable Jinja macros directly in the manifest. diff --git a/docs/roadmap.md b/docs/roadmap.md index ce43ba64..d1354ad2 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -88,7 +88,7 @@ configurations with variables, control flow, and custom functions. - [ ] **Dynamic Features and Custom Functions:** - - [ ] Implement support for basic Jinja control structures (`{% if %}` and + - [x] Implement support for basic Jinja control structures (`{% if %}` and `{% for %}`) - [ ] Implement the foreach key for target generation. diff --git a/src/ast.rs b/src/ast.rs index fe85b0e9..5fd3fcc9 100644 --- a/src/ast.rs +++ b/src/ast.rs @@ -19,6 +19,10 @@ use semver::Version; use serde::{Deserialize, Serialize, de::Deserializer}; +use std::collections::HashMap; + +/// Map type for `vars` blocks, preserving YAML values. +pub type Vars = HashMap; fn deserialize_actions<'de, D>(deserializer: D) -> Result, D::Error> where @@ -30,7 +34,6 @@ where } Ok(actions) } -use std::collections::HashMap; /// Top-level manifest structure parsed from a `Netsukefile`. /// @@ -61,7 +64,7 @@ pub struct NetsukeManifest { /// Global key/value pairs available to recipes. #[serde(default)] - pub vars: HashMap, + pub vars: Vars, /// Named rule templates that can be referenced by targets. #[serde(default)] @@ -187,7 +190,7 @@ pub struct Target { /// Target-scoped variables available during command execution. #[serde(default)] - pub vars: HashMap, + pub vars: Vars, /// Declares that the target does not correspond to a real file. #[serde(default)] diff --git a/src/manifest.rs b/src/manifest.rs index 23edb6ca..6df3b1d5 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -5,9 +5,9 @@ //! They wrap `serde_yml` and add basic file handling. use crate::ast::NetsukeManifest; -use anyhow::{Context, Result}; +use anyhow::{Context, Result, bail}; use minijinja::{Environment, UndefinedBehavior, context, value::Value}; -use std::{collections::HashMap, fs, path::Path}; +use std::{fs, path::Path}; /// Parse a manifest string using Jinja for templating. /// @@ -48,22 +48,16 @@ pub fn from_str(yaml: &str) -> Result { let doc: serde_yml::Value = serde_yml::from_str(&rendered).context("first-pass YAML parse error")?; - let vars = doc - .get("vars") - .and_then(|v| v.as_mapping()) - .map(|m| { - m.iter() - .filter_map(|(k, v)| k.as_str().and_then(|key| v.as_str().map(|val| (key, val)))) - .map(|(k, v)| (k.to_string(), v.to_string())) - .collect::>() - }) - .unwrap_or_default(); - - // Populate the environment with the extracted variables for subsequent - // rendering. Undefined variables now trigger errors to surface template - // mistakes early. - for (key, value) in vars { - env.add_global(key, Value::from(value)); + if let Some(vars) = doc.get("vars").and_then(|v| v.as_mapping()) { + // Copy each key-value pair into the environment, preserving native YAML + // types so control structures like `{% if %}` and `{% for %}` can operate + // on booleans and sequences. + for (k, v) in vars { + let Some(key) = k.as_str() else { + bail!("non-string key in 'vars' mapping: {k:?}"); + }; + env.add_global(key, Value::from_serialize(v.clone())); + } } env.set_undefined_behavior(UndefinedBehavior::Strict); diff --git a/tests/data/jinja_for.yml b/tests/data/jinja_for.yml new file mode 100644 index 00000000..ad4f213e --- /dev/null +++ b/tests/data/jinja_for.yml @@ -0,0 +1,11 @@ +netsuke_version: 1.0.0 +vars: + items: + - foo + - bar +targets: +{% for item in items %} + - name: "{{ item }}" + command: "echo {{ item }}" +{% endfor %} + diff --git a/tests/data/jinja_for_invalid.yml b/tests/data/jinja_for_invalid.yml new file mode 100644 index 00000000..cc86f552 --- /dev/null +++ b/tests/data/jinja_for_invalid.yml @@ -0,0 +1,9 @@ +netsuke_version: 1.0.0 +vars: + items: 1 +targets: +{% for item in items %} + - name: "{{ item }}" + command: "echo {{ item }}" +{% endfor %} + diff --git a/tests/data/jinja_if.yml b/tests/data/jinja_if.yml new file mode 100644 index 00000000..416880b3 --- /dev/null +++ b/tests/data/jinja_if.yml @@ -0,0 +1,6 @@ +netsuke_version: 1.0.0 +vars: + enable: true +targets: + - name: hello + command: "{% if enable %}echo on{% else %}echo off{% endif %}" diff --git a/tests/data/jinja_if_disabled.yml b/tests/data/jinja_if_disabled.yml new file mode 100644 index 00000000..7a9c2c2e --- /dev/null +++ b/tests/data/jinja_if_disabled.yml @@ -0,0 +1,6 @@ +netsuke_version: 1.0.0 +vars: + enable: false +targets: + - name: hello + command: "{% if enable %}echo on{% else %}echo off{% endif %}" diff --git a/tests/features/manifest.feature b/tests/features/manifest.feature index 042212a1..f19be71f 100644 --- a/tests/features/manifest.feature +++ b/tests/features/manifest.feature @@ -55,3 +55,29 @@ Feature: Manifest Parsing Given the manifest file "tests/data/jinja_undefined.yml" is parsed When the parsing result is checked Then parsing the manifest fails + + Scenario: Rendering Jinja conditionals in a manifest + Given the manifest file "tests/data/jinja_if.yml" is parsed + When the manifest is checked + Then the first target name is "hello" + And the first target command is "echo on" + + Scenario: Rendering Jinja conditionals in a manifest (disabled) + Given the manifest file "tests/data/jinja_if_disabled.yml" is parsed + When the manifest is checked + Then the first target name is "hello" + And the first target command is "echo off" + + Scenario: Rendering Jinja loops in a manifest + Given the manifest file "tests/data/jinja_for.yml" is parsed + When the manifest is checked + Then the manifest has 2 targets + And the target 1 name is "foo" + And the target 1 command is "echo foo" + And the target 2 name is "bar" + And the target 2 command is "echo bar" + + Scenario: Parsing fails when a Jinja loop iterates over a non-list + Given the manifest file "tests/data/jinja_for_invalid.yml" is parsed + When the parsing result is checked + Then parsing the manifest fails diff --git a/tests/manifest_jinja_tests.rs b/tests/manifest_jinja_tests.rs index 245b05f2..824399cf 100644 --- a/tests/manifest_jinja_tests.rs +++ b/tests/manifest_jinja_tests.rs @@ -3,18 +3,17 @@ use netsuke::{ast::Recipe, manifest}; use rstest::rstest; +fn manifest_yaml(body: &str) -> String { + format!("netsuke_version: 1.0.0\n{body}") +} + #[rstest] fn renders_global_vars() { - let yaml = r" -netsuke_version: 1.0.0 -vars: - who: world -targets: - - name: hello - command: echo {{ who }} -"; - - let manifest = manifest::from_str(yaml).expect("parse"); + let yaml = manifest_yaml( + "vars:\n who: world\ntargets:\n - name: hello\n command: echo {{ who }}\n", + ); + + let manifest = manifest::from_str(&yaml).expect("parse"); let first = manifest.targets.first().expect("target"); if let Recipe::Command { command } = &first.recipe { assert_eq!(command, "echo world"); @@ -25,24 +24,96 @@ targets: #[rstest] fn undefined_variable_errors() { - let yaml = r" -netsuke_version: 1.0.0 -targets: - - name: hello - command: echo {{ missing }} -"; + let yaml = manifest_yaml("targets:\n - name: hello\n command: echo {{ missing }}\n"); - assert!(manifest::from_str(yaml).is_err()); + assert!(manifest::from_str(&yaml).is_err()); } #[rstest] fn syntax_error_errors() { - let yaml = r" -netsuke_version: 1.0.0 -targets: - - name: hello - command: echo {{ who -"; - - assert!(manifest::from_str(yaml).is_err()); + let yaml = manifest_yaml("targets:\n - name: hello\n command: echo {{ who\n"); + + assert!(manifest::from_str(&yaml).is_err()); +} + +#[rstest] +#[case(true, "echo on")] +#[case(false, "echo off")] +fn renders_if_blocks(#[case] flag: bool, #[case] expected: &str) { + let cmd = "{% if flag %}echo on{% else %}echo off{% endif %}"; + let yaml = manifest_yaml(&format!( + concat!( + "vars:\n", + " flag: {flag}\n", + "targets:\n", + " - name: test\n", + " command: {cmd}\n", + ), + flag = flag, + cmd = cmd, + )); + + let manifest = manifest::from_str(&yaml).expect("parse"); + let first = manifest.targets.first().expect("target"); + if let Recipe::Command { command } = &first.recipe { + assert_eq!(command, expected); + } else { + panic!("Expected command recipe, got: {:?}", first.recipe); + } +} + +#[rstest] +fn renders_for_loops() { + let yaml = manifest_yaml( + "vars:\n items:\n - a\n - b\ntargets:\n{% for item in items %}\n - name: \"{{ item }}\"\n command: \"echo {{ item }}\"\n{% endfor %}\n", + ); + + let manifest = manifest::from_str(&yaml).expect("parse"); + assert_eq!(manifest.targets.len(), 2); + let names: Vec<_> = manifest + .targets + .iter() + .map(|t| match &t.name { + netsuke::ast::StringOrList::String(s) => s.clone(), + other => panic!("Expected String, got: {other:?}"), + }) + .collect(); + assert_eq!(names, vec!["a", "b"]); + + let commands: Vec<_> = manifest + .targets + .iter() + .map(|t| match &t.recipe { + Recipe::Command { command } => command.clone(), + other => panic!("Expected command recipe, got: {other:?}"), + }) + .collect(); + assert_eq!(commands, vec!["echo a", "echo b"]); +} + +#[rstest] +fn for_loop_non_iterable_errors() { + let yaml = manifest_yaml( + "vars:\n items: 1\ntargets:\n{% for item in items %}\n - name: \"{{ item }}\"\n command: \"echo {{ item }}\"\n{% endfor %}\n", + ); + + assert!(manifest::from_str(&yaml).is_err()); +} + +#[rstest] +fn undefined_in_if_errors() { + let yaml = manifest_yaml( + "targets:\n - name: test\n command: {% if missing %}echo hi{% endif %}\n", + ); + + assert!(manifest::from_str(&yaml).is_err()); +} + +#[rstest] +fn undefined_in_for_errors() { + let yaml = manifest_yaml( + "targets:\n{% for item in missing %}\n - name: \"{{ item }}\"\n command: \"echo {{ item }}\"\n{% endfor %}\n", + ); + + assert!(manifest::from_str(&yaml).is_err()); } diff --git a/tests/steps/manifest_steps.rs b/tests/steps/manifest_steps.rs index af99bcb0..448a5a95 100644 --- a/tests/steps/manifest_steps.rs +++ b/tests/steps/manifest_steps.rs @@ -1,8 +1,4 @@ -//! Step definitions for manifest parsing scenarios. -#![expect( - clippy::needless_pass_by_value, - reason = "Cucumber requires owned String arguments" -)] +//! Step definitions for manifest feature tests. use crate::CliWorld; use cucumber::{given, then, when}; @@ -27,28 +23,40 @@ fn parse_manifest_inner(world: &mut CliWorld, path: &str) { fn assert_manifest(world: &CliWorld) { assert!( world.manifest.is_some(), - "manifest should have been parsed successfully" + "manifest should have been parsed successfully", ); } fn assert_parsed(world: &CliWorld) { assert!( world.manifest.is_some() || world.manifest_error.is_some(), - "manifest should have been parsed" + "manifest should have been parsed", ); } #[given(expr = "the manifest file {string} is parsed")] +#[expect( + clippy::needless_pass_by_value, + reason = "Cucumber step requires owned String" +)] fn given_parse_manifest(world: &mut CliWorld, path: String) { parse_manifest_inner(world, &path); } #[when(expr = "the manifest file {string} is parsed")] +#[expect( + clippy::needless_pass_by_value, + reason = "Cucumber step requires owned String" +)] fn parse_manifest(world: &mut CliWorld, path: String) { parse_manifest_inner(world, &path); } #[when(regex = r"^the (?Pparsing result|manifest|version|flags|rules) (?:is|are) checked$")] +#[expect( + clippy::needless_pass_by_value, + reason = "Cucumber step requires owned String" +)] fn when_item_checked(world: &mut CliWorld, item: String) { match item.as_str() { "parsing result" => assert_parsed(world), @@ -58,19 +66,22 @@ fn when_item_checked(world: &mut CliWorld, item: String) { } #[then(expr = "the manifest version is {string}")] +#[expect( + clippy::needless_pass_by_value, + reason = "Cucumber step requires owned String" +)] fn manifest_version(world: &mut CliWorld, version: String) { let manifest = world.manifest.as_ref().expect("manifest"); assert_eq!(manifest.netsuke_version.to_string(), version); } #[then(expr = "the first target name is {string}")] +#[expect( + clippy::needless_pass_by_value, + reason = "Cucumber step requires owned String" +)] fn first_target_name(world: &mut CliWorld, name: String) { - let manifest = world.manifest.as_ref().expect("manifest"); - let first = manifest.targets.first().expect("targets"); - match &first.name { - StringOrList::String(value) => assert_eq!(value, &name), - other => panic!("Expected StringOrList::String, got: {other:?}"), - } + assert_target_name(world, 1, &name); } #[then("the first target is phony")] @@ -100,6 +111,10 @@ fn manifest_parse_error(world: &mut CliWorld) { } #[then(expr = "the first rule name is {string}")] +#[expect( + clippy::needless_pass_by_value, + reason = "Cucumber step requires owned String" +)] fn first_rule_name(world: &mut CliWorld, name: String) { let manifest = world.manifest.as_ref().expect("manifest"); let rule = manifest.rules.first().expect("rules"); @@ -107,12 +122,59 @@ fn first_rule_name(world: &mut CliWorld, name: String) { } #[then(expr = "the first target command is {string}")] +#[expect( + clippy::needless_pass_by_value, + reason = "Cucumber step requires owned String" +)] fn first_target_command(world: &mut CliWorld, command: String) { + assert_target_command(world, 1, &command); +} + +#[then(expr = "the manifest has {int} targets")] +fn manifest_has_targets(world: &mut CliWorld, count: usize) { let manifest = world.manifest.as_ref().expect("manifest"); - let first = manifest.targets.first().expect("targets"); - if let Recipe::Command { command: actual } = &first.recipe { - assert_eq!(actual, &command); + assert_eq!(manifest.targets.len(), count); +} + +fn assert_target_name(world: &CliWorld, index: usize, name: &str) { + let manifest = world.manifest.as_ref().expect("manifest"); + let target = manifest + .targets + .get(index - 1) + .unwrap_or_else(|| panic!("missing target {index}")); + match &target.name { + StringOrList::String(value) => assert_eq!(value, name), + other => panic!("Expected StringOrList::String, got: {other:?}"), + } +} + +fn assert_target_command(world: &CliWorld, index: usize, command: &str) { + let manifest = world.manifest.as_ref().expect("manifest"); + let target = manifest + .targets + .get(index - 1) + .unwrap_or_else(|| panic!("missing target {index}")); + if let Recipe::Command { command: actual } = &target.recipe { + assert_eq!(actual, command); } else { - panic!("Expected command recipe, got: {:?}", first.recipe); + panic!("Expected command recipe, got: {:?}", target.recipe); } } + +#[then(expr = "the target {int} name is {string}")] +#[expect( + clippy::needless_pass_by_value, + reason = "Cucumber step requires owned String" +)] +fn target_name_n(world: &mut CliWorld, index: usize, name: String) { + assert_target_name(world, index, &name); +} + +#[then(expr = "the target {int} command is {string}")] +#[expect( + clippy::needless_pass_by_value, + reason = "Cucumber step requires owned String" +)] +fn target_command_n(world: &mut CliWorld, index: usize, command: String) { + assert_target_command(world, index, &command); +}