-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
refactor(form_mapping.go): mapping multipart request #1829
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1829 +/- ##
==========================================
- Coverage 98.64% 98.64% -0.01%
==========================================
Files 41 41
Lines 2148 2144 -4
==========================================
- Hits 2119 2115 -4
Misses 18 18
Partials 11 11
Continue to review full report at Codecov.
|
binding/form.go
Outdated
multipartFileHeaderStructType = reflect.TypeOf(multipart.FileHeader{}) | ||
) | ||
|
||
func (r *multipartRequest) Set(value reflect.Value, field reflect.StructField, key string, opt setOptions) (isSetted bool, err error) { |
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.
check: var _ setter = &multipartRequest
and add node for export method/class/interface.
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.
What do you mean when you talk about "node"?
Btw, this method is not exported because the type multipartRequest
is not exported, and I see no warnings about this from any linters.
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.
@vkd sorry, word typo, note
.
Yes, it's not exported. We should add some usage note for new and important interface/method/class even if un-exported. thanks!
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.
Ok, thanks. I've added a couple of comments.
binding/form_mapping.go
Outdated
|
||
type formSource map[string][]string | ||
|
||
func (form formSource) Set(value reflect.Value, field reflect.StructField, tagValue string, opt setOptions) (isSetted bool, err error) { |
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.
check: var _ setter = &formSource
and add note for export method/class/interface.
return mappingByPtr(ptr, formSource(form), tag) | ||
} | ||
|
||
type setter interface { |
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.
add some notes?
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.
please not use // setter -
style, thanks!
binding/form.go
Outdated
@@ -72,7 +72,8 @@ var ( | |||
multipartFileHeaderStructType = reflect.TypeOf(multipart.FileHeader{}) | |||
) | |||
|
|||
func (r *multipartRequest) Set(value reflect.Value, field reflect.StructField, key string, opt setOptions) (isSetted bool, err error) { | |||
// TrySet - try to set a value by the multipart request with the binding a form file |
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.
please not use TrySet -
style, thanks! like: TrySet tries to set ...
} | ||
|
||
type formSource map[string][]string | ||
|
||
var _ setter = formSource(nil) | ||
|
||
func (form formSource) Set(value reflect.Value, field reflect.StructField, tagValue string, opt setOptions) (isSetted bool, err error) { | ||
// TrySet - try to set a value by request's form source (like map[string][]string) | ||
func (form formSource) TrySet(value reflect.Value, field reflect.StructField, tagValue string, opt setOptions) (isSetted bool, err error) { |
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.
the same above, thanks!
Refactor mapping to interface
setter
- for supporting a multipart request.Benchmarks of a simple form mapping (with many fields and one field respectively):