feat(template): make join template function more flexible#5233
Open
TheMeier wants to merge 1 commit into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (2)
📝 WalkthroughWalkthroughThe ChangesJoin Function Reflection Support
Estimated code review effort: 3 (Moderate) | ~20 minutes 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
alt v is invalid
JoinFunc-->>TemplateCaller: return ("", nil)
else v is []string
JoinFunc->>StringsJoin: join strings with sep
StringsJoin-->>JoinFunc: concatenated string
JoinFunc-->>TemplateCaller: return result
else v is slice or array
JoinFunc->>Reflect: iterate elements
Reflect->>FmtSprint: convert elements to strings
FmtSprint-->>JoinFunc: element strings
JoinFunc-->>TemplateCaller: return joined result
else v is unsupported
JoinFunc-->>TemplateCaller: return error
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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
📥 Commits
Reviewing files that changed from the base of the PR and between 369a175 and 9d60bb794842f049a3750ae41670514d7676395d.
📒 Files selected for processing (3)
docs/notifications.mdtemplate/template.gotemplate/template_test.go
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?