feat(template): make join template function more flexible#5233
Open
TheMeier wants to merge 1 commit into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe ChangesJoin Function Reflection Support
Sequence Diagram(s)sequenceDiagram
participant TemplateCaller
participant JoinFunc as join(sep, v)
participant Reflect as reflect
participant StringsJoin as strings.Join
participant FmtSprint as fmt.Sprint
TemplateCaller->>JoinFunc: invoke with sep + v
JoinFunc->>Reflect: inspect v (isValid, kind)
alt v is invalid
JoinFunc-->>TemplateCaller: return ("", nil)
else v is []string
JoinFunc->>StringsJoin: strings.Join(v, sep)
StringsJoin-->>JoinFunc: concatenated string
JoinFunc-->>TemplateCaller: return (result, nil)
else v is slice/array
JoinFunc->>Reflect: iterate elements
Reflect->>FmtSprint: convert each element
FmtSprint-->>JoinFunc: element strings
JoinFunc->>JoinFunc: join elements with sep
JoinFunc-->>TemplateCaller: return (result, nil)
else
JoinFunc-->>TemplateCaller: return ("", error)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/notifications.md`:
- Line 92: Update the docs for the join filter to explicitly state coercion
behavior: clarify that join (described as "join") accepts elements of type any
and will convert non-string elements to their string representation (e.g. via
standard stringification like fmt.Sprint) before concatenation, and that it does
not return an error on conversion (unless the implementation documents
otherwise); reference the existing strings.Join mention for string-only behavior
and note the argument order inversion for templates.
In `@template/template_test.go`:
- Around line 355-359: Add a test case in template_test.go that exercises join
with non-string elements to ensure elements are stringified: add a case similar
to the existing "Template using join with list" entry but with in set to `{{
list 1 true "x" | join "," }}` and exp set to `1,true,x`; place it alongside the
other cases in the same test table so the join behavior for mixed-type lists is
enforced when the test runner iterates over the cases.
In `@template/template.go`:
- Around line 201-204: The join implementation currently enforces elements be
strings by asserting elem.(string) and returning an error; change it to accept
any element type by removing the string type assertion and instead convert each
elem to a string (e.g. via fmt.Sprint or fmt.Sprintf("%v", elem)) before
concatenation so join accepts slices/arrays of any element type; update the code
in the join function where the variable elem is handled to perform this
conversion and remove the error path that checks for non-string types.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c597f3de-18ea-4227-9cb6-c49c470ec3fa
📒 Files selected for processing (3)
docs/notifications.mdtemplate/template.gotemplate/template_test.go
| | dict | values ...any | Returns a map of string to any, constructed from the variadic list of key-value pairs. The number of arguments must be even, and the keys must be strings. | | ||
| | humanizeDuration | number or string | Returns a human-readable string representing the duration, and the error if it happened. | | ||
| | join | sep string, s []string | [strings.Join](http://golang.org/pkg/strings/#Join), concatenates the elements of s to create a single string. The separator string sep is placed between elements in the resulting string. (note: argument order inverted for easier pipelining in templates.) | | ||
| | join | sep string, any | [strings.Join](http://golang.org/pkg/strings/#Join), concatenates the elements of the provided slice to create a single string. The separator string sep is placed between elements in the resulting string. (note: argument order inverted for easier pipelining in templates.) | |
There was a problem hiding this comment.
Document coercion behavior explicitly for join
Line 92 updates the argument type to any, but the description should explicitly say non-string elements are converted to strings before joining (and when errors occur, if any). That avoids ambiguous API expectations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/notifications.md` at line 92, Update the docs for the join filter to
explicitly state coercion behavior: clarify that join (described as "join")
accepts elements of type any and will convert non-string elements to their
string representation (e.g. via standard stringification like fmt.Sprint) before
concatenation, and that it does not return an error on conversion (unless the
implementation documents otherwise); reference the existing strings.Join mention
for string-only behavior and note the argument order inversion for templates.
Allow `join` template function to accept a slice of any type, not just strings. The function will convert non-string elements to their string representation before joining them. This provides more flexibility in templating, e.g. creating slices with `list` and then joining them. Signed-off-by: Christoph Maser <christoph.maser+github@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Checklist
Please check all the applicable boxes.
Which user-facing changes does this PR introduce?
Summary by CodeRabbit
New Features
Documentation
Tests