-
Notifications
You must be signed in to change notification settings - Fork 4
fix(context): throw ErrViewNotFound when no view is avaiable #37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's Guide by SourceryThis pull request fixes an issue where the absence of a view would cause the application to panic. Instead of triggering a panic, the code now properly handles the situation by checking for the ErrViewNotFound error and responding with a 404 status. The changes are implemented by adding error checks in both the application handler and the context, as well as defining a new error indicator for missing views. Sequence diagram for ErrViewNotFound error handlingsequenceDiagram
participant Client
participant App
participant Context
Client->>App: Request view
App->>Context: Call View(data, options)
Context-->>App: Return ErrViewNotFound (if no view available)
App->>Client: Write 404 status with "View Not Found" response
Class diagram for Error Handling EnhancementclassDiagram
class App {
+HandleFile(name, v)
}
class Context {
+View(data, options)
-Routing: Routing
}
class Routing {
+Viewers: []
}
class ErrViewNotFound
Context --> Routing : uses
Context ..> ErrViewNotFound : returns
App ..> ErrViewNotFound : checks for error
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @cnlangzi - I've reviewed your changes - here's some feedback:
Overall Comments:
- When writing the 404 response, set the appropriate Content-Type header (e.g. text/plain) to clearly communicate the response type.
- Consider handling the error from ctx.Response.Write rather than suppressing it with nolint to avoid silent failures.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 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.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #37 +/- ##
==========================================
+ Coverage 91.89% 91.93% +0.04%
==========================================
Files 38 38
Lines 1407 1414 +7
==========================================
+ Hits 1293 1300 +7
Misses 78 78
Partials 36 36
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Here's the code health analysis summary for commits Analysis Summary
|
Changed
Fixed
Added
Tests
Tasks to complete before merging PR:
make unit-test
to check for any regressions 📋make lint
to check for any issuesSummary by Sourcery
Bug Fixes: