-
Notifications
You must be signed in to change notification settings - Fork 0
refactor: New() returns (*Limiter, error) instead of hiding errors #1
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
knownbots.New() errors are now ignored with fallback to nil validator instead of panicking, allowing the application to start gracefully
- New() signature changed from New() *Limiter to New() (*Limiter, error) - knownbots.New() errors are properly returned to caller - Updated all callers to handle error return value - Updated README.md examples with error handling BREAKING CHANGE: New() now returns (limiter, error), callers must check error
Reviewer's GuideRefactors Limiter construction so New returns (*Limiter, error) instead of panicking on knownbots initialization failure, and updates tests, examples, and documentation to handle the new error-aware API and slightly simplify Allow usage in the README. Sequence diagram for Limiter creation with error-aware NewsequenceDiagram
actor App
participant Botrate as Botrate_New
participant Knownbots as Knownbots_New
App->>Botrate: New(opts...)
activate Botrate
Botrate->>Knownbots: New()
alt Knownbots initialization succeeds
Knownbots-->>Botrate: validator, nil error
Botrate-->>App: limiter, nil error
else Knownbots initialization fails
Knownbots-->>Botrate: nil, error
Botrate-->>App: nil, error
end
deactivate Botrate
Class diagram for updated Limiter construction via NewclassDiagram
class Config
class Limiter {
-cfg Config
}
class Botrate {
+New(opts Option) *Limiter, error
}
class Knownbots {
+New() Validator, error
}
Botrate ..> Limiter
Botrate ..> Knownbots
Limiter ..> Knownbots
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Benchmark Results |
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 - I've found 1 issue, and left some high level feedback:
- The tests now have a lot of repeated
l, err := New(...)/if err != nil { t.Fatalf(...) }boilerplate; consider introducing a small test helper likenewLimiterTB(tb testing.TB, opts ...Option) *Limiterto centralize error handling and keep individual tests focused on behavior. - The README example handler changed from using
result.Reasonto a generic429 Too Many Requestsfor all failures; if this alters the intended public behavior (distinguishing fake bots vs rate limiting), consider either preserving the richer responses or adding a separate example to demonstrate the more detailed handling.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The tests now have a lot of repeated `l, err := New(...)` / `if err != nil { t.Fatalf(...) }` boilerplate; consider introducing a small test helper like `newLimiterTB(tb testing.TB, opts ...Option) *Limiter` to centralize error handling and keep individual tests focused on behavior.
- The README example handler changed from using `result.Reason` to a generic `429 Too Many Requests` for all failures; if this alters the intended public behavior (distinguishing fake bots vs rate limiting), consider either preserving the richer responses or adding a separate example to demonstrate the more detailed handling.
## Individual Comments
### Comment 1
<location> `botrate_test.go:13-15` </location>
<code_context>
func TestLimiter_New(t *testing.T) {
- l := New()
+ l, err := New()
+
+ if err != nil {
+ t.Fatalf("New() returned error: %v", err)
+ }
</code_context>
<issue_to_address>
**issue (testing):** Add a test that covers the failure case where New() returns a non-nil error
Current tests only cover the success path of `New()` and verify that it returns no error. To validate the new behavior when `New()` fails (e.g., when `knownbots.New()` returns an error), add a test that forces this failure and asserts `New()` returns `nil, err` instead of panicking. You may need light dependency injection or a test-only hook around the knownbots constructor to simulate the failure.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
Summary by Sourcery
Change limiter construction to return errors from New instead of panicking and update callers accordingly.
Enhancements:
Documentation:
Tests:
Chores: