refactor: replace utils package with core functions in template engines#446
refactor: replace utils package with core functions in template engines#446ReneWerner87 merged 5 commits intomasterfrom
Conversation
ReneWerner87
requested review from
efectn,
gaby and
sixcolors
and removed request for
a team
ReneWerner87
added
🤖 Dependencies
Summary of ChangesHello @ReneWerner87, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the internal structure of the template engine implementations by consolidating common file reading and directory traversal utilities within the main Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a well-executed refactoring to decouple the various template engines from the gofiber/utils package. By centralizing the file system operations (ReadFile and Walk) into the core template package and providing them as wrapper functions, the individual engines now have a cleaner dependency tree. The changes are consistently applied across all template engine implementations, and the go.mod and go.sum files are updated accordingly. This is a solid improvement for the project's modularity and maintainability.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (19)
ace/go.modis excluded by!**/*.modace/go.sumis excluded by!**/*.sum,!**/*.sumamber/go.modis excluded by!**/*.modamber/go.sumis excluded by!**/*.sum,!**/*.sumdjango/go.modis excluded by!**/*.moddjango/go.sumis excluded by!**/*.sum,!**/*.sumgo.modis excluded by!**/*.modhandlebars/go.modis excluded by!**/*.modhandlebars/go.sumis excluded by!**/*.sum,!**/*.sumhtml/go.modis excluded by!**/*.modhtml/go.sumis excluded by!**/*.sum,!**/*.sumjet/go.modis excluded by!**/*.modjet/go.sumis excluded by!**/*.sum,!**/*.summustache/go.modis excluded by!**/*.modmustache/go.sumis excluded by!**/*.sum,!**/*.sumpug/go.modis excluded by!**/*.modpug/go.sumis excluded by!**/*.sum,!**/*.sumslim/go.modis excluded by!**/*.modslim/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (11)
.gitignoreace/ace.goamber/amber.godjango/django.gohandlebars/handlebars.gohtml/html.gojet/jet.gomustache/mustache.gopug/pug.goslim/slim.gotemplate.go
🧰 Additional context used
🧬 Code graph analysis (6)
jet/jet.go (1)
template.go (1)
ReadFile(176-178)
ace/ace.go (1)
template.go (2)
ReadFile(176-178)Walk(184-186)
html/html.go (1)
template.go (2)
ReadFile(176-178)Walk(184-186)
mustache/mustache.go (1)
template.go (2)
ReadFile(176-178)Walk(184-186)
pug/pug.go (1)
template.go (2)
ReadFile(176-178)Walk(184-186)
slim/slim.go (1)
template.go (2)
ReadFile(176-178)Walk(184-186)
🪛 GitHub Actions: Golangci-Lint
template.go
[error] 11-11: could not import github.com/gofiber/utils/v2 (template.go:11:2: missing go.sum entry for module providing package github.com/gofiber/utils/v2 (imported by github.com/gofiber/template/v2); to add:)
🪛 GitHub Actions: Govulncheck Security Scan
template.go
[error] 11-11: govulncheck failed: missing go.sum entry for module providing package github.com/gofiber/utils/v2 (imported by github.com/gofiber/template/v2); to add:
🪛 GitHub Check: golangci-lint
template.go
[failure] 185-185:
undefined: utils (typecheck)
[failure] 177-177:
undefined: utils (typecheck)
🪛 GitHub Check: govulncheck-check
template.go
[failure] 11-11:
could not import github.com/gofiber/utils/v2 (invalid package name: "")
[failure] 11-11:
missing go.sum entry for module providing package github.com/gofiber/utils/v2 (imported by github.com/gofiber/template/v2); to add:
[failure] 185-185:
undefined: utils
[failure] 177-177:
undefined: utils
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Agent
- GitHub Check: Compare
- GitHub Check: Tests (1.25.x, windows-latest)
🔇 Additional comments (18)
.gitignore (1)
19-19: LGTM!Adding
.claudeto the ignore list is appropriate for excluding tool-specific files from version control.template.go (1)
173-186: Good architectural design with clear documentation.The wrapper functions provide a clean abstraction layer, allowing template engines to use file operations without directly depending on the utils package. The documentation clearly explains the purpose of these wrappers.
Once the dependency issue is resolved, these implementations will work correctly.
html/html.go (1)
100-100: LGTM! Clean migration to core package functions.The replacement of
utils.ReadFileandutils.Walkwith theircorepackage equivalents is correct and maintains the existing logic. The error handling and control flow remain unchanged.Also applies to: 123-123
pug/pug.go (1)
97-97: LGTM! Consistent refactoring pattern.The migration from
utilstocorepackage functions follows the same clean pattern as other template engines. The file operations maintain their existing behavior and error handling.Also applies to: 128-128
handlebars/handlebars.go (1)
93-93: LGTM! Refactoring completes the migration pattern.The Handlebars engine successfully adopts the
corepackage functions, completing the consistent refactoring across all template engines in this PR.Also applies to: 113-113
amber/amber.go (2)
107-107: LGTM! Refactored to use core.ReadFile.The replacement of utils.ReadFile with core.ReadFile is correct and maintains the same functionality through the wrapper function in template.go.
133-133: LGTM! Refactored to use core.Walk.The replacement of utils.Walk with core.Walk is correct and maintains the same functionality through the wrapper function in template.go.
django/django.go (2)
122-122: LGTM! Refactored to use core.ReadFile.The replacement is consistent with the refactoring pattern applied across all template engines.
144-144: LGTM! Refactored to use core.Walk.The replacement is consistent with the refactoring pattern applied across all template engines.
ace/ace.go (2)
101-101: LGTM! Refactored to use core.ReadFile.The replacement is correct and aligns with the unified file I/O interface provided by the core package.
137-137: LGTM! Refactored to use core.Walk.The replacement is correct and aligns with the unified file I/O interface provided by the core package.
jet/jet.go (1)
126-126: LGTM! Refactored to use core.ReadFile.The replacement is correct. Note that this engine appropriately continues using
filepath.Walkdirectly at line 143 since it uses an InMemLoader rather than the FileSystem-based walking pattern.mustache/mustache.go (3)
32-32: LGTM! Refactored to use core.ReadFile.The replacement in the partial provider is correct and maintains the same functionality for loading partial templates.
111-111: LGTM! Refactored to use core.ReadFile.The replacement is consistent with the refactoring pattern applied across all template engines.
139-139: LGTM! Refactored to use core.Walk.The replacement is consistent with the refactoring pattern applied across all template engines.
slim/slim.go (3)
13-13: LGTM! Import aligns with refactoring objective.The core package alias correctly references the template package that provides the centralized file operation wrappers.
88-88: LGTM! ReadFile call is correctly updated.The refactoring properly uses the core package wrapper, maintaining identical behavior while centralizing the file operation interface.
120-120: LGTM! Walk call is correctly updated.The refactoring properly uses the core package wrapper with the correct parameter order and types. Verified consistency across all template engines (ace, amber, django, handlebars, html, jet, mustache, pug): all are correctly using core.Walk and core.ReadFile with proper imports.
There was a problem hiding this comment.
Pull request overview
This PR refactors template engine implementations to use wrapper functions in the core template package instead of directly importing the utils package. The refactor centralizes utils function calls (ReadFile and Walk) in template.go, with all template engines (slim, pug, mustache, jet, html, handlebars, django, amber, ace) updated to call these wrapper functions via the core package.
Key Changes
- Added ReadFile and Walk wrapper functions to template.go that forward calls to utils/v2 package
- Updated all template engines to replace
utils.ReadFilewithcore.ReadFileandutils.Walkwithcore.Walk - Migrated from utils v1.2.0 to utils/v2 v2.0.0-rc.5, moving it from direct to indirect dependency in engine modules
Reviewed changes
Copilot reviewed 20 out of 30 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| template.go | Added ReadFile and Walk wrapper functions that delegate to utils/v2 package |
| slim/slim.go | Replaced utils.ReadFile and utils.Walk calls with core.ReadFile and core.Walk |
| pug/pug.go | Replaced utils.ReadFile and utils.Walk calls with core.ReadFile and core.Walk |
| mustache/mustache.go | Replaced utils.ReadFile and utils.Walk calls with core.ReadFile and core.Walk |
| jet/jet.go | Replaced utils.ReadFile call with core.ReadFile |
| html/html.go | Replaced utils.ReadFile and utils.Walk calls with core.ReadFile and core.Walk |
| handlebars/handlebars.go | Replaced utils.ReadFile and utils.Walk calls with core.ReadFile and core.Walk |
| django/django.go | Replaced utils.ReadFile and utils.Walk calls with core.ReadFile and core.Walk |
| amber/amber.go | Replaced utils.ReadFile and utils.Walk calls with core.ReadFile and core.Walk |
| ace/ace.go | Replaced utils.ReadFile and utils.Walk calls with core.ReadFile and core.Walk |
| go.mod | Added utils/v2 v2.0.0-rc.5 as direct dependency; specifies Go version 1.25 |
| */go.mod | Updated engine modules to have utils/v2 as indirect dependency instead of direct |
| */go.sum | Updated checksums for utils/v2 and its transitive dependencies |
| .gitignore | Added .claude to ignored files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
template.go (1)
173-183: Simplify the error handling.The explicit error check is redundant since you're just forwarding the result from
utils.ReadFile. Directly returning the call result simplifies the code.🔎 Proposed simplification
func ReadFile(path string, fs http.FileSystem) ([]byte, error) { - buf, err := utils.ReadFile(path, fs) - if err != nil { - return nil, err - } - return buf, nil + return utils.ReadFile(path, fs) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (1)
template.go
🧰 Additional context used
🪛 GitHub Actions: Golangci-Lint
template.go
[error] 191-191: golangci-lint (revive): if-return: redundant if ...; err != nil check, just return error instead.
🪛 GitHub Check: golangci-lint
template.go
[failure] 192-192:
error returned from external package is unwrapped: sig: func github.com/gofiber/utils/v2.Walk(fs net/http.FileSystem, root string, walkFn path/filepath.WalkFunc) error (wrapcheck)
[failure] 180-180:
error returned from external package is unwrapped: sig: func github.com/gofiber/utils/v2.ReadFile(path string, fs net/http.FileSystem) ([]byte, error) (wrapcheck)
[failure] 191-191:
if-return: redundant if ...; err != nil check, just return error instead. (revive)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Compare
- GitHub Check: Tests (1.25.x, windows-latest)
🔇 Additional comments (1)
template.go (1)
7-7: LGTM: Necessary imports for the new wrapper functions.The
osimport is required for theos.FileInfotype in theWalkfunction signature, and theutils/v2import is needed for delegating to the underlying implementation.Also applies to: 11-11
…dling in ReadFile and Walk functions
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Makefile (1)
19-24: Consider cross-platform compatibility for opening the coverage report.The
opencommand on Line 24 is macOS-specific. On Linux, usexdg-open; on Windows, usestart. Consider either documenting this limitation or using a cross-platform solution.🔎 Example cross-platform approach
Option 1: Remove the
opencommand and document that users should opencoverage.htmlmanually:coverage: go run gotest.tools/gotestsum@v1.12.0 -f testname -- ./... -race -count=1 -coverprofile=coverage.out -covermode=atomic go tool cover -html=coverage.out -o coverage.html - open coverage.html & + @echo "Coverage report generated: coverage.html"Option 2: Add OS detection:
coverage: go run gotest.tools/gotestsum@v1.12.0 -f testname -- ./... -race -count=1 -coverprofile=coverage.out -covermode=atomic go tool cover -html=coverage.out -o coverage.html - open coverage.html & + @case "$$(uname -s)" in \ + Darwin) open coverage.html & ;; \ + Linux) xdg-open coverage.html & ;; \ + MINGW*|MSYS*) start coverage.html & ;; \ + esactemplate.go (1)
186-196: Consider refactoring to address linter feedback on error handling pattern.The linter flagged the
if err := ...; err != nilpattern in past reviews. While the logic is correct, you can satisfy the linter by splitting the assignment from the condition check:🔎 Alternative pattern to address linter feedback
func Walk(fs http.FileSystem, directory string, walkFn func(path string, info os.FileInfo, err error) error) error { - if err := utils.Walk(fs, directory, walkFn); err != nil { + err := utils.Walk(fs, directory, walkFn) + if err != nil { return fmt.Errorf("failed to walk directory: %w", err) } return nil }This maintains the error wrapping while using a pattern that linters typically prefer.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Makefiletemplate.go
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 52-52: Missing required phony target "all"
(minphony)
[warning] 52-52: Missing required phony target "clean"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Tests (1.25.x, windows-latest)
- GitHub Check: Compare
🔇 Additional comments (2)
template.go (2)
4-13: LGTM!The import additions (fmt, os) are appropriate for the new wrapper functions that format errors and handle file info types.
174-184: LGTM!The ReadFile wrapper correctly delegates to utils.ReadFile and properly wraps errors with context while preserving the error chain using
%w.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.