fix(viewengine): continue to load other files when one file fails #69
Conversation
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
Reviewer's Guide by SourceryThe pull request modifies the view engine loading process to continue loading files even if one file fails to load. This is achieved by changing the Load methods in the view engines to not return errors and instead log them. The individual load methods for components, layouts, pages, and views in the HTML view engine were also modified to log errors instead of returning them. Sequence diagram for loading HTML templates with error handlingsequenceDiagram
participant HtmlViewEngine
participant fsys as FileSystem
participant app as App
HtmlViewEngine->>fsys: WalkDir("components", ...)
loop For each component file
fsys->>HtmlViewEngine: Returns path to file
HtmlViewEngine->>HtmlViewEngine: loadTemplate(path)
alt Error loading template
HtmlViewEngine->>app: app.logger.Error("xun: load html", path, err)
end
end
HtmlViewEngine->>fsys: WalkDir("layouts", ...)
loop For each layout file
fsys->>HtmlViewEngine: Returns path to file
HtmlViewEngine->>HtmlViewEngine: loadTemplate(path)
alt Error loading template
HtmlViewEngine->>app: app.logger.Error("xun: load html", path, err)
end
end
HtmlViewEngine->>fsys: WalkDir("pages", ...)
loop For each page file
fsys->>HtmlViewEngine: Returns path to file
HtmlViewEngine->>HtmlViewEngine: loadPage(path)
alt Error loading page
HtmlViewEngine->>app: app.logger.Error("xun: load html", path, err)
end
end
HtmlViewEngine->>fsys: WalkDir("views", ...)
loop For each view file
fsys->>HtmlViewEngine: Returns path to file
HtmlViewEngine->>HtmlViewEngine: loadView(path)
alt Error loading view
HtmlViewEngine->>app: app.logger.Error("xun: load html", path, err)
end
end
Updated class diagram for TextViewEngineclassDiagram
class TextViewEngine {
-templates map[string]*TextTemplate
-fsys fs.FS
-app *App
+Load(fsys fs.FS, app *App)
-loadText(path string) error
}
note for TextViewEngine "Load method no longer returns an error. Errors are now logged internally."
Updated class diagram for StaticViewEngineclassDiagram
class StaticViewEngine {
+Load(fsys fs.FS, app *App)
+FileChanged(fsys fs.FS, app *App, event fsnotify.Event) error
-handle(fsys fs.FS, app *App, path string)
}
note for StaticViewEngine "Load method no longer returns an error."
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Here's the code health analysis summary for commits Analysis Summary
|
There was a problem hiding this comment.
Hey @cnlangzi - I've reviewed your changes - here's some feedback:
Overall Comments:
- The
Loadmethods no longer return errors, so the error variable in the walk functions is now unused and can be dropped. - Consider adding a recover statement to the goroutines to prevent panics from crashing the application.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #69 +/- ##
==========================================
+ Coverage 91.23% 91.28% +0.04%
==========================================
Files 62 62
Lines 2545 2490 -55
==========================================
- Hits 2322 2273 -49
+ Misses 192 188 -4
+ Partials 31 29 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@sourcery-ai review |
There was a problem hiding this comment.
Pull Request Overview
This PR modifies the view engine loading mechanism to ensure that a single file loading failure does not prevent the rest of the files from being processed. Key changes include:
- Removing the error propagation from the Load functions in TextViewEngine, StaticViewEngine, and HtmlViewEngine.
- Logging errors for individual file failures using slog instead of stopping the entire loading process.
- Adjusting the ViewEngine interface and corresponding mock implementations in tests to reflect the new Load signature.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| viewengine_text.go | Removed error returns from Load; now logs errors per file load. |
| viewengine_static.go | Updated Load to remove error propagation and log errors. |
| viewengine_html.go | Changed Load and its helper functions to log errors instead of returning them. |
| viewengine.go | Updated the ViewEngine interface to use a void Load method. |
| app_watch_test.go | Adjusted mock implementations to reflect updated Load signature. |
| app.go | Updated engines loading to call the new void Load method. |
There was a problem hiding this comment.
Hey @cnlangzi - I've reviewed your changes - here's some feedback:
Overall Comments:
- The nolint comments should have a reason why the error is being ignored.
- Consider adding a method to the ViewEngine interface to return the name of the engine for logging purposes.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the resilience of the view engine by modifying the Load methods to log errors instead of stopping file processing when an individual file fails to load. Key changes include:
- Removing error returns from Load methods in TextViewEngine, StaticViewEngine, and HtmlViewEngine.
- Logging errors for file-load failures within WalkDir callbacks.
- Updating test and interface definitions to align with the new error-handling design.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| viewengine_text.go | Updated Load method to log errors instead of returning them. |
| viewengine_static.go | Modified Load to ignore errors on file loading in the public folder. |
| viewengine_html.go | Adjusted Load and its helper methods to internalize and log errors. |
| viewengine.go | Updated the ViewEngine interface to remove the error return from Load. |
| app_watch_test.go | Updated mock to reflect new Load signature. |
| app_test.go | Tweaked HTTP transport DialContext to target a local address. |
| app.go | Removed error handling from Load call for each view engine. |
Comments suppressed due to low confidence (2)
viewengine_html.go:113
- Ensure there are unit tests verifying that errors during layout loading are correctly logged since the function no longer returns an error.
fs.WalkDir(ve.fsys, "layouts", func(path string, d fs.DirEntry, _ error) error { // nolint: errcheck
viewengine.go:13
- Add tests to verify that any file load errors are properly handled and logged by each view engine, given that the Load method no longer returns an error.
Load(fsys fs.FS, app *App)
There was a problem hiding this comment.
Hey @cnlangzi - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a comment to the
fs.WalkDiranonymous functions to explain why the error is being ignored. - The
FileChangedmethod onmockViewEngineshould probably not return an unhandled error.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -1,8 +1,8 @@ | |||
| package xun | |||
There was a problem hiding this comment.
issue (complexity): Consider creating a common helper function to encapsulate the FS walk logic and error logging, reducing code duplication across the load functions for components, layouts, pages, and views, and improving maintainability and readability..
You’re repeating four nearly identical FS walks with custom callbacks. Consolidating them into a common helper would reduce duplication and overall complexity. For example, you can create a helper method to encapsulate the walking logic and error logging, then pass a function to process matching files. For instance:
func (ve *HtmlViewEngine) walkAndProcess(dir string, process func(path string) error) {
fs.WalkDir(ve.fsys, dir, func(path string, d fs.DirEntry, _ error) error { // nolint: errcheck
if !strings.EqualFold(filepath.Ext(path), ".html") {
return nil
}
if err := process(path); err != nil {
ve.app.logger.Error("xun: load html", slog.String("path", path), slog.Any("err", err))
}
return nil
})
}Then update your functions like loadComponents, loadLayouts, loadViews, and even loadPages (with slight adjustments if needed) to use this helper:
func (ve *HtmlViewEngine) loadComponents() {
ve.walkAndProcess("components", func(path string) error {
_, err := ve.loadTemplate(path)
return err
})
}
func (ve *HtmlViewEngine) loadLayouts() {
ve.walkAndProcess("layouts", func(path string) error {
_, err := ve.loadTemplate(path)
return err
})
}
func (ve *HtmlViewEngine) loadViews() {
ve.walkAndProcess("views", func(path string) error {
return ve.loadView(path)
})
}
// Adjust loadPages accordingly if needed:
func (ve *HtmlViewEngine) loadPages() {
ve.walkAndProcess("pages", func(path string) error {
return ve.loadPage(path)
})
}This refactoring removes repetitive error handling and inline logging while keeping all functionality intact.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Changed
Fixed
Added
Tests
Tasks to complete before merging PR:
make unit-teststo check for any regressions 📋make lintto check for any issuesSummary by Sourcery
Modify view engine loading to continue processing files even when individual file loading fails, improving error handling and resilience
Bug Fixes:
Enhancements: