From f0800b8e13f294e134e6173bf14a377e5b67bdc8 Mon Sep 17 00:00:00 2001 From: Spencer Brower <6729162+sbrow@users.noreply.github.com> Date: Fri, 7 Aug 2026 10:48:04 -0400 Subject: [PATCH] feat: Added diagnostics to `first` and `last`. --- DIAGNOSTIC_TODOS.yaml | 36 +++++++++++++++++++ TODOS.md | 17 ++++++--- mustache/mustache.odin | 8 ++--- mustache/pipes.odin | 75 +++++++++++++++++++++++++++++++++++----- mustache/pipes_test.odin | 4 +-- 5 files changed, 120 insertions(+), 20 deletions(-) diff --git a/DIAGNOSTIC_TODOS.yaml b/DIAGNOSTIC_TODOS.yaml index ba9fc2e..9116ca1 100644 --- a/DIAGNOSTIC_TODOS.yaml +++ b/DIAGNOSTIC_TODOS.yaml @@ -229,3 +229,39 @@ expected: | Show where the error was triggered. Should also show the format that will be used actual: | [ERROR] --- [333:apply_format()] format pipe used but no date format configured (set date.format in thor.json) Default will be used +--- +name: first/last with zero argument +input: | + +expected: Error with source location — "argument must not be zero" +actual: | + [ERROR] --- [render_template()] argument must not be zero + --> :: + | + N | + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +--- +name: first/last with negative argument +input: | + +expected: Warning with source location — "first -3 is equivalent to last 3 — consider using the clearer form" +actual: | + [WARN ] --- [take()] first -3 is equivalent to last 3 + --> :: + | + N |
    {{#items | first -3}}
  • {{.}}
    • {{/items}}
+ | ^^^^^^^^^^^^^^^^^^^^^^^^^ consider using the clearer form + | +--- +name: first/last on string with no argument +input: | + {{word | first}} +expected: Warning with source location — "defaulting to n=1 for string; consider specifying n explicitly" +actual: | + [WARN ] --- [take()] defaulting to n=1 for string + --> :: + | + N | {{word | first}} + | ^^^^^^^^^^^^^^^ consider specifying n explicitly + | diff --git a/TODOS.md b/TODOS.md index eaf0d73..51abae5 100644 --- a/TODOS.md +++ b/TODOS.md @@ -17,12 +17,19 @@ Remember to polish existing features before moving on to new ones. - current: `layouts/partials` - alt1: `layouts`, `partials` - alt2: `templates/layouts`, `templates/partials` +- [ ] add `sort_by` pipe - [ ] pipes - - [ ] first/last - - [ ] warn when using negative numbers - - [ ] warn when using default arg on string - - [ ] add the above warnings to DIAGNOSTIC_TODOS.md - - [ ] sort_by + - [x] first/last + - [ ] write test for error when n == 0 + - [x] warn when using negative numbers + - [x] warn when using default arg on string + - [x] add the above warnings to DIAGNOSTIC_TODOS.md + - [ ] Instead of "clearer form", be exact: "last 5" + - [ ] write tests for `first 0` & `last 0` + - [ ] update tests to assert diagnostics when appropriate + - do we want to do this or just use DIAGNOSTIC_TODOS.md as the tests? + - [ ] also update to use file logger so they don't clog up the output + - [ ] narrow highlight in diagnostic to relevent part of line. ## Documentation diff --git a/mustache/mustache.odin b/mustache/mustache.odin index 097cd17..9bced01 100644 --- a/mustache/mustache.odin +++ b/mustache/mustache.odin @@ -621,7 +621,7 @@ render_nodes :: proc( warn_unknown_key(current, ctx[:], node) } if len(node.filters) > 0 { - transformed, perr := apply_pipeline(val, node.filters[:], node.pos, ctx[:]) + transformed, perr := apply_pipeline(val, node.filters[:], node.pos, ctx[:], current) if perr != nil { return tag_error(perr, current) } @@ -640,7 +640,7 @@ render_nodes :: proc( warn_unknown_key(current, ctx[:], node) } if len(node.filters) > 0 { - transformed, perr := apply_pipeline(val, node.filters[:], node.pos, ctx[:]) + transformed, perr := apply_pipeline(val, node.filters[:], node.pos, ctx[:], current) if perr != nil { return tag_error(perr, current) } @@ -655,7 +655,7 @@ render_nodes :: proc( warn_unknown_key(current, ctx[:], node) } if len(node.filters) > 0 { - transformed, perr := apply_pipeline(val, node.filters[:], node.pos, ctx[:]) + transformed, perr := apply_pipeline(val, node.filters[:], node.pos, ctx[:], current) if perr != nil { return tag_error(perr, current) } @@ -701,7 +701,7 @@ render_nodes :: proc( warn_unknown_key(current, ctx[:], node) } if len(node.filters) > 0 { - transformed, perr := apply_pipeline(val, node.filters[:], node.pos, ctx[:]) + transformed, perr := apply_pipeline(val, node.filters[:], node.pos, ctx[:], current) if perr != nil { return tag_error(perr, current) } diff --git a/mustache/pipes.odin b/mustache/pipes.odin index 5b37179..36f7962 100644 --- a/mustache/pipes.odin +++ b/mustache/pipes.odin @@ -220,13 +220,14 @@ apply_pipeline :: proc( filters: []Pipe_Filter, pos: int, ctx: []any, + tmpl: Template, ) -> ( current: any, err: Error, ) { current = value for &filter in filters { - current = apply_filter(current, &filter, pos, ctx) or_return + current = apply_filter(current, &filter, pos, ctx, tmpl) or_return log.debugf("applied: filter=%v before=%s after=%s pos=%d", filter, value, current, pos) } return @@ -256,7 +257,16 @@ resolve_format_string :: proc(name: string, ctx: []any, pos: int) -> (string, Er return str, nil } -apply_filter :: proc(value: any, filter: ^Pipe_Filter, pos: int, ctx: []any) -> (any, Error) { +apply_filter :: proc( + value: any, + filter: ^Pipe_Filter, + pos: int, + ctx: []any, + tmpl: Template, +) -> ( + any, + Error, +) { op, ok := pipe_op_from_string(filter.op) if !ok { hint := "" @@ -315,9 +325,9 @@ apply_filter :: proc(value: any, filter: ^Pipe_Filter, pos: int, ctx: []any) -> return any{new_clone(str2, context.temp_allocator), typeid_of(string)}, nil } case .First: - return take(value, filter.args[:], pos, from_start = true) + return take(value, filter.args[:], true, tmpl, pos, filter.op_pos) case .Last: - return take(value, filter.args[:], pos, from_start = false) + return take(value, filter.args[:], false, tmpl, pos, filter.op_pos) } return {}, nil } @@ -414,7 +424,17 @@ apply_group_by :: proc(value: any, args: []string, pos: int) -> (result: any, er return groups, nil } -take :: proc(value: any, args: []string, pos: int, from_start: bool) -> (any, Error) { +take :: proc( + value: any, + args: []string, + from_start: bool, + tmpl: Template, + tag_pos: int, + op_pos: int, +) -> ( + any, + Error, +) { elem_info, count, data := list_info(value) is_list := elem_info != nil from_start := from_start @@ -423,7 +443,13 @@ take :: proc(value: any, args: []string, pos: int, from_start: bool) -> (any, Er if !is_list { s, is_str := reflect.as_string(value) if !is_str { - return nil, Error_Body{msg = "requires a list or string", pos = pos, kind = .Data} + return nil, Error_Body { + msg = "requires a list or string", + pos = tag_pos, + kind = .Data, + source = tmpl.source, + path = tmpl.path, + } } str = s count = len(str) @@ -431,23 +457,54 @@ take :: proc(value: any, args: []string, pos: int, from_start: bool) -> (any, Er n := 1 if !is_list && len(args) == 0 { - return nil, Error_Body{msg = "requires an argument for strings", pos = pos, kind = .Data} + path := tmpl.path != "" ? tmpl.path : "" + diag := format_tag_error( + path, + tmpl.source, + op_pos, + "defaulting to n=1 for string", + "consider specifying n explicitly", + ) + log.warnf("%s", diag) } if len(args) > 0 { parsed, ok := strconv.parse_int(args[0]) if !ok { return nil, Error_Body { msg = fmt.tprintf("invalid argument '%s'", args[0]), - pos = pos, + pos = tag_pos, kind = .Data, + source = tmpl.source, + path = tmpl.path, } } n = parsed } + if n == 0 { + return nil, Error_Body { + msg = "argument must not be zero", + pos = tag_pos, + kind = .Data, + source = tmpl.source, + path = tmpl.path, + } + } + if n < 0 { + op_name := from_start ? "first" : "last" + alt_name := from_start ? "last" : "first" + path := tmpl.path != "" ? tmpl.path : "" + diag := format_tag_error( + path, + tmpl.source, + op_pos, + fmt.tprintf("%s %d is equivalent to %s %d", op_name, n, alt_name, 0 - n), + "consider using the clearer form", + ) + log.warnf("%s", diag) from_start = !from_start - n = 0 - n // abs(n) + n = 0 - n } start: int diff --git a/mustache/pipes_test.odin b/mustache/pipes_test.odin index 133c7d5..2ae68a9 100644 --- a/mustache/pipes_test.odin +++ b/mustache/pipes_test.odin @@ -572,7 +572,7 @@ test_unknown_pipe_op_suggestion :: proc(t: ^testing.T) { op = "formats", op_pos = 0, } - _, err := apply_filter("2026-01-15", &filter, 0, nil) + _, err := apply_filter("2026-01-15", &filter, 0, nil, {}) testing.expect(t, err != nil, "should error on unknown op") b := body(err) testing.expect(t, strings.contains(b.hint, "format"), "hint should suggest 'format'") @@ -585,7 +585,7 @@ test_unknown_pipe_op_no_suggestion :: proc(t: ^testing.T) { op = "xyz", op_pos = 0, } - _, err := apply_filter("2026-01-15", &filter, 0, nil) + _, err := apply_filter("2026-01-15", &filter, 0, nil, {}) testing.expect(t, err != nil, "should error on unknown op") b := body(err) testing.expect(t, b.hint == "", "no suggestion expected for 'xyz'")