Proposal: Remove Must and Should when binding structs #3178
Conversation
if err is nil err.Error() panics (eg. c.Bind().Must().JSON(...) successfully binds but panics
make sure returnErr works with nil error
as in majority of cases we expect err to be nil, this should provide better short-cutting
ItsMeSamey
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
WalkthroughThe pull request implements significant changes to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@efectn please review whenever possible |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
bind.go (1)
43-43: Update method documentationThe comment above the
Custommethod still mentions "Should/Must is still valid for Custom binders" which is no longer accurate since this functionality has been removed.-// NOTE: Should/Must is still valid for Custom binders. +// Binds data using a custom binder registered with the Fiber instance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- bind.go (11 hunks)
- bind_test.go (0 hunks)
- ctx.go (0 hunks)
💤 Files with no reviewable changes (2)
- bind_test.go
- ctx.go
🧰 Additional context used
🔇 Additional comments (8)
bind.go (8)
22-22: LGTM: Simplified Bind structThe removal of the
shouldfield aligns with the PR objective to remove Must/Should functionality, resulting in a cleaner and simpler struct definition.
Line range hint
52-56: LGTM: Clean error handling implementationThe error handling is now more straightforward with direct returns, while maintaining proper struct validation.
61-65: LGTM: Consistent error handlingThe implementation follows the same pattern as other binding methods, maintaining consistency across the codebase.
Line range hint
71-75: LGTM: Proper error handling with validationThe implementation correctly handles errors from the cookie binder and maintains struct validation.
Line range hint
80-84: LGTM: Consistent implementation across all binding methodsAll binding methods (Query, JSON, XML, Form, URI) follow the same pattern of direct error handling and validation, maintaining consistency and simplifying the codebase.
Also applies to: 89-93, 98-102, 107-111, 116-120
Line range hint
125-129: LGTM: Proper multipart form handlingThe implementation correctly handles multipart form binding with appropriate error handling and validation.
147-147: LGTM: Proper custom binder handlingThe implementation correctly handles custom binder parsing with direct error returns, consistent with the new error handling approach.
Line range hint
1-165: Verify complete removal of Must/Should functionalityLet's verify that all references to Must/Should have been properly removed from the codebase.
|
@ItsMeSamey we have decided to keep the feature to fill the response automatically thank you for bringing this topic to our attention we can keep your other PR and expand it if necessary here, however, it should only be decided whether the response may be prefilled with the error so we want to rename this functionality and document it better, and you can help us with this |
Bugs Fixed
Fixes issue described in #3171
Changes introduced
Removed
Must,Shouldand related code and tests