From c550d49b063cf4cf2ef25b1bc460af976629a07e Mon Sep 17 00:00:00 2001 From: zakstucke <44890343+zakstucke@users.noreply.github.com> Date: Tue, 20 Feb 2024 19:05:02 +0200 Subject: [PATCH] Fixed bug when using custom_extensions and initials together, made lockfile alphabetical to prevent annoying git diffs when nothing changed (#13) --- py_rust/src/config/engine.rs | 13 +++---- py_rust/src/render/lockfile.rs | 2 + py_rust/src/render/mod.rs | 3 +- py_rust/src/utils/mod.rs | 2 + py_rust/src/utils/ordered_map_serializer.rs | 19 ++++++++++ .../tests/render/test_custom_extensions.py | 38 ++++++++++++++++++- py_rust/tests/render/test_lockfile.py | 26 +++++++++++++ 7 files changed, 93 insertions(+), 10 deletions(-) create mode 100644 py_rust/src/utils/ordered_map_serializer.rs diff --git a/py_rust/src/config/engine.rs b/py_rust/src/config/engine.rs index 47a9a16..ed357cb 100644 --- a/py_rust/src/config/engine.rs +++ b/py_rust/src/config/engine.rs @@ -199,14 +199,13 @@ impl Engine { Ok::<_, error_stack::Report>(()) })?; - // Consume current contents of PY_USER_FUNCS and add to minijinja env: - let mut custom_funcs_global = PY_USER_FUNCS.lock(); - - // Extract all the loaded funcs from the global mutex to be passed to individual closures: - let custom_funcs = std::mem::take(&mut *custom_funcs_global); - *custom_funcs_global = HashMap::new(); - + // Extract a copy of the user funcs to add to minijinja env: + // Note copying as env might be created multiple times (e.g. initial) + // TODO: instead of this maybe reusing an env? In general need a bit of a refactor! + let custom_funcs = PY_USER_FUNCS.lock().clone(); for (name, py_fn) in custom_funcs.into_iter() { + debug!("Registering custom function: '{}'", name); + // Confirm doesn't clash with config var: if conf.context.contains_key(&name) { return Err(zerr!( diff --git a/py_rust/src/render/lockfile.rs b/py_rust/src/render/lockfile.rs index 00984a8..be90dd3 100644 --- a/py_rust/src/render/lockfile.rs +++ b/py_rust/src/render/lockfile.rs @@ -15,6 +15,8 @@ pub static LOCKFILE_NAME: &str = ".zetch.lock"; #[derive(Debug, serde::Serialize, serde::Deserialize)] struct Contents { version: String, + // Keep ordering in lockfile static to reduce git conflicts and diff noise: + #[serde(serialize_with = "crate::utils::ordered_map_serializer")] files: HashMap, } diff --git a/py_rust/src/render/mod.rs b/py_rust/src/render/mod.rs index 9d2ec9a..ae65bc9 100644 --- a/py_rust/src/render/mod.rs +++ b/py_rust/src/render/mod.rs @@ -4,7 +4,6 @@ use bitbazaar::{ }; use colored::Colorize; use minijinja::context; -use tracing::{debug, info, warn}; mod args_validate; mod debug; @@ -81,7 +80,7 @@ pub fn render(args: &crate::args::Args, render_args: &RenderCommand) -> Result( + value: &HashMap, + serializer: S, +) -> Result +where + S: Serializer, +{ + let ordered: BTreeMap<_, _> = value.iter().collect(); + ordered.serialize(serializer) +} diff --git a/py_rust/tests/render/test_custom_extensions.py b/py_rust/tests/render/test_custom_extensions.py index 29cb319..89e1e8e 100644 --- a/py_rust/tests/render/test_custom_extensions.py +++ b/py_rust/tests/render/test_custom_extensions.py @@ -411,6 +411,41 @@ def test_custom_ext_nice_user_invalid_errs( ) +def test_custom_ext_multi_render(): + """Check custom extensions don't break when renderer runs multiple times (e.g. when cli var initials used). + + This test was made for a real bug where extensions were lost after the first renderer usage. + """ + with TmpFileManager() as manager: + ext = manager.tmpfile( + """import zetch +@zetch.register_function +def capitalize(s): + return s.capitalize() +""", + suffix=".py", + ) + + check_single( + manager, + manager.create_cfg( + { + "engine": {"custom_extensions": [str(ext)]}, + "context": { + "cli": { + "var_with_initial": { + "commands": ["echo bar"], + "initial": "init", + } + } + }, + } + ), + "{{ capitalize('foo') }} {{ var_with_initial }}", + "Foo bar", + ) + + # DONE duplicate pkg names/sys paths # DONE conflict with ctx/in built filter/in built function # DONE check module and importing between files works @@ -434,7 +469,8 @@ def test_custom_ext_nice_user_invalid_errs( # DONE allow input as string instead of file, that should print to stdout for read, write, del # DONE probably remove most engine config, maybe making top level if minimal enough, we don't want to mess with files and error early (so enforce no_undefined and keep_trailing_newline) # DONE fix schema - not sure why its not working -# TODO: order everything in the lockfile to prevent diffs when nothings actually changed. +# DONE: order everything in the lockfile to prevent diffs when nothings actually changed. +# TODO: Before thinking about modes/light etc as other solutions to circular deps. I think by default the repeated rendering should be improved to be a first class citizen, then, every run, static+env rendered in first + initials when not existing in lockfile + others clis as empty strings + custom extensions as empty strings, only then a second render with cli commands, this could get rid of a bunch of circular dep problems natively. # TODO some sort of heavy/light/modes solution to caching values and not recomputing, maybe also for ban-defaults etc. maybe a modes top level config section, where a mode can override any config, set ban-defaults etc, need to think, but also need a way to only run certain post and pre in certain modes, need to think on best api. # TODO think about interop with jinja,cookiecutter,copier,etc # TODO decide and document optimal formatting, probably using scolvins and making sure it can working with custom extensions. diff --git a/py_rust/tests/render/test_lockfile.py b/py_rust/tests/render/test_lockfile.py index 5a95a56..82c0a80 100644 --- a/py_rust/tests/render/test_lockfile.py +++ b/py_rust/tests/render/test_lockfile.py @@ -1,4 +1,6 @@ +import collections import json +import uuid from pathlib import Path import pytest @@ -155,3 +157,27 @@ def test_lockfile_only_write_when_needed(): ), }, } + + +def test_lockfile_deterministic_ordering(): + """Make sure the lockfile always serializes object keys in alphabetical order. + + This makes git diffs on the lockfile much less likely to conflict, and only show changes when real. + """ + with TmpFileManager() as manager: + filenames = [ + str(manager.tmpfile(content="Hello!", full_name=f"{uuid.uuid4()}.zetch.txt").name) + for _ in range(10) + ] + cli.render( + manager.root_dir, + manager.create_cfg({}), + ) + with open(get_lockfile_path(manager.root_dir), "r") as file: + # In cur python think will be ordered by default, but older pythons might not be, + # so use ordered dict in json decoder to be sure order unaffected by loading into python: + lock = json.load(file, object_pairs_hook=collections.OrderedDict) + assert lock["version"] == zetch.__version__ + + # They should come out in alphabetical order: + assert [k for k in lock["files"]] == sorted(filenames)