Optimize code adjust #2700

daheige · 2021-04-22T13:45:31Z

1.setFormMap error of result
2.adjust code for TrySet
3.error export for type multipart.FileHeader
4.code style adjust
5.reflect code maping optimize

codecov · 2021-04-22T13:56:06Z

Codecov Report

Merging #2700 (7e656c6) into master (6703dea) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2700   +/-   ##
=======================================
  Coverage   98.69%   98.69%           
=======================================
  Files          41       41           
  Lines        2074     2074           
=======================================
  Hits         2047     2047           
  Misses         15       15           
  Partials       12       12

Impacted Files	Coverage Δ
binding/header.go	`100.00% <ø> (ø)`
render/json.go	`85.29% <ø> (ø)`
binding/form_mapping.go	`100.00% <100.00%> (ø)`
binding/multipart_form_mapping.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6703dea...7e656c6. Read the comment docs.

appleboy · 2021-04-28T05:03:35Z

binding/form_mapping.go

+var (
+	errUnknownType = errors.New("unknown type")
+
+	// ErrConvertMapStringSlice covert to map[string][]string


replace covert to map[string][]string with can not covert to map[string][]string

This is not what you said, this is the error returned, as a developer using gin can know the error of the underlying transformation, rather than what you said not to let others know, which is a bad design

// ErrConvertMapStringSlice covert to map[string][]string // ErrConvertToMapString can not convert to map[string]string

I don't know why the format can't be the same between ErrConvertMapStringSlice and ErrConvertToMapString?

I mean that comment message.

errors.New("can not convert to map of strings")

comment message:

// can not convert to map[string]string

but

errors.New("can not convert to map slices of strings")

comment message:

// covert to map[string][]string

not

// can not convert to map[string][]string

OK, if there are two errors here, they should be separated. As for the comment information you said, you can modify it appropriately. For the function point here, I hope the caller knows why the error occurred, instead of returning as before.

I mean that comment message.

errors.New("can not convert to map of strings")

comment message:

// can not convert to map[string]string

but

errors.New("can not convert to map slices of strings")

comment message:

// covert to map[string][]string

not

// can not convert to map[string][]string

You can change the notes here. Thank you very much

I just want to change the comment

-// ErrConvertMapStringSlice covert to map[string][]string +// ErrConvertMapStringSlice can not covert to map[string][]string

thinkerou · 2021-04-28T10:40:40Z

binding/form_mapping.go

+	ErrConvertMapStringSlice = errors.New("can not convert to map slices of strings")
+
+	// ErrConvertToMapString can not convert to map[string]string
+	ErrConvertToMapString = errors.New("can not convert to map of strings")


not output var

If there is an error in the conversion, it should be made known to the caller, not hidden in the current gin framework. I don't think your gin team has done a very good job of handling the error, and the way it is handled is not very friendly to the caller. I think there is something wrong with the design. Maybe more Go developers feel the same way.

appleboy · 2021-04-28T23:15:38Z

Why created the other two PR #2709 and #2708 ?

daheige · 2021-04-29T00:55:36Z

If the previous proposal can be improved in the future, I will close this one. Thank you Original ------------------ From: Bo-Yi Wu ***@***.***> Date: Thu,Apr 29,2021 7:15 AM To: gin-gonic/gin ***@***.***> Cc: heige ***@***.***>, Author ***@***.***> Subject: Re: [gin-gonic/gin] Optimize code adjust (#2700) Why created the other two PR #2709 and #2708 ? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

daheige · 2021-04-29T03:26:43Z

There are two ways to deal with the previous error. I just extract the error from the above. You can see the previous processing. If the same error is the case, you can use an error variable.

…

------------------ Original ------------------ From: Bo-Yi Wu ***@***.***> Date: Thu,Apr 29,2021 10:46 AM To: gin-gonic/gin ***@***.***> Cc: heige ***@***.***>, Author ***@***.***> Subject: Re: [gin-gonic/gin] Optimize code adjust (#2700) @appleboy commented on this pull request. In binding/form_mapping.go: > @@ -16,7 +16,15 @@ import ( "github.com/gin-gonic/gin/internal/json" ) -var errUnknownType = errors.New("unknown type") +var ( +errUnknownType = errors.New("unknown type") + +// ErrConvertMapStringSlice covert to map[string][]string // ErrConvertMapStringSlice covert to map[string][]string // ErrConvertToMapString can not convert to map[string]string I don't know why the format can't be the same between ErrConvertMapStringSlice and ErrConvertToMapString? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

daheige · 2021-04-29T05:24:52Z

good,thanks，you can change it，no problem.

…

------------------ Original ------------------ From: Bo-Yi Wu ***@***.***> Date: Thu,Apr 29,2021 1:06 PM To: gin-gonic/gin ***@***.***> Cc: heige ***@***.***>, Author ***@***.***> Subject: Re: [gin-gonic/gin] Optimize code adjust (#2700) @appleboy commented on this pull request. In binding/form_mapping.go: > @@ -16,7 +16,15 @@ import ( "github.com/gin-gonic/gin/internal/json" ) -var errUnknownType = errors.New("unknown type") +var ( +errUnknownType = errors.New("unknown type") + +// ErrConvertMapStringSlice covert to map[string][]string I just want to change the comment -// ErrConvertMapStringSlice covert to map[string][]string +// ErrConvertMapStringSlice can not covert to map[string][]string — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

daheige · 2021-04-29T05:30:03Z

done.

…

------------------ Original ------------------ From: Bo-Yi Wu ***@***.***> Date: Thu,Apr 29,2021 1:06 PM To: gin-gonic/gin ***@***.***> Cc: heige ***@***.***>, Author ***@***.***> Subject: Re: [gin-gonic/gin] Optimize code adjust (#2700) @appleboy commented on this pull request. In binding/form_mapping.go: > @@ -16,7 +16,15 @@ import ( "github.com/gin-gonic/gin/internal/json" ) -var errUnknownType = errors.New("unknown type") +var ( +errUnknownType = errors.New("unknown type") + +// ErrConvertMapStringSlice covert to map[string][]string I just want to change the comment -// ErrConvertMapStringSlice covert to map[string][]string +// ErrConvertMapStringSlice can not covert to map[string][]string — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

thinkerou

lgtm

daheige added 5 commits April 22, 2021 21:43

setFormMap error of result

39c4287

adjust code for TrySet

20d21ff

error export for type multipart.FileHeader

6661885

code style adjust

523761d

reflect code maping optimize

5eb55a8

appleboy requested changes Apr 28, 2021

View reviewed changes

appleboy added the enhancement label Apr 28, 2021

appleboy added this to the v1.8 milestone Apr 28, 2021

thinkerou reviewed Apr 28, 2021

View reviewed changes

Update form_mapping.go

8728ba2

appleboy approved these changes Apr 29, 2021

View reviewed changes

Merge branch 'master' into optimize-code-adjust

f767157

appleboy requested a review from thinkerou April 30, 2021 23:13

Merge branch 'master' into optimize-code-adjust

2e79f15

thinkerou approved these changes Jun 1, 2021

View reviewed changes

Merge branch 'master' into optimize-code-adjust

7e656c6

thinkerou merged commit 97a32b1 into gin-gonic:master Jun 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize code adjust #2700

Optimize code adjust #2700

daheige commented Apr 22, 2021

codecov bot commented Apr 22, 2021 •

edited

Loading

appleboy Apr 28, 2021

daheige Apr 28, 2021 •

edited

Loading

appleboy Apr 29, 2021

appleboy Apr 29, 2021

daheige Apr 29, 2021

daheige Apr 29, 2021

appleboy Apr 29, 2021

thinkerou Apr 28, 2021

daheige Apr 28, 2021 •

edited

Loading

appleboy commented Apr 28, 2021

daheige commented Apr 29, 2021 via email

daheige commented Apr 29, 2021 via email

daheige commented Apr 29, 2021 via email

daheige commented Apr 29, 2021 via email

thinkerou left a comment

Optimize code adjust #2700

Optimize code adjust #2700

Conversation

daheige commented Apr 22, 2021

codecov bot commented Apr 22, 2021 • edited Loading

Codecov Report

appleboy Apr 28, 2021

Choose a reason for hiding this comment

daheige Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

appleboy Apr 29, 2021

Choose a reason for hiding this comment

appleboy Apr 29, 2021

Choose a reason for hiding this comment

daheige Apr 29, 2021

Choose a reason for hiding this comment

daheige Apr 29, 2021

Choose a reason for hiding this comment

appleboy Apr 29, 2021

Choose a reason for hiding this comment

thinkerou Apr 28, 2021

Choose a reason for hiding this comment

daheige Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

appleboy commented Apr 28, 2021

daheige commented Apr 29, 2021 via email

daheige commented Apr 29, 2021 via email

daheige commented Apr 29, 2021 via email

daheige commented Apr 29, 2021 via email

thinkerou left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 22, 2021 •

edited

Loading

daheige Apr 28, 2021 •

edited

Loading

daheige Apr 28, 2021 •

edited

Loading