Skip to content
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

support default value for form #1138

Merged
merged 11 commits into from Apr 25, 2018
Merged

support default value for form #1138

merged 11 commits into from Apr 25, 2018

Conversation

thinkerou
Copy link
Member

@thinkerou thinkerou commented Oct 24, 2017

ref #1052


TODOs:

  • test case using this new api

TODO(add cases) have completed, but it need #1168 merged firstly.

@thinkerou thinkerou closed this Oct 24, 2017
@thinkerou thinkerou reopened this Oct 24, 2017
@javierprovecho javierprovecho added this to the 1.3 milestone Oct 24, 2017
inputFieldName = inputFieldNameList[0]
var defaultValue interface{}
if len(inputFieldNameList) > 1 {
defaultList := strings.Split(inputFieldNameList[1], "=")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe use strings.SplitN(inputFieldNameList[1], "=", 2) and remove the condition below len(defaultList) == 2 . as it is right now, is also ok.

https://golang.org/pkg/strings/#SplitN

Copy link
Member Author

@thinkerou thinkerou Oct 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we should use SplitN which can reduce some codes(interface{} and len(defaultList)==2). Thx!

@@ -23,6 +24,15 @@ func mapForm(ptr interface{}, form map[string][]string) error {

structFieldKind := structField.Kind()
inputFieldName := typeField.Tag.Get("form")
inputFieldNameList := strings.Split(inputFieldName, ",")
inputFieldName = inputFieldNameList[0]
var defaultValue interface{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any need to cast it as interface{}? As I see below, you'll always use it as string and parse it to the specific type, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, use string, thanks!

Copy link
Member

@javierprovecho javierprovecho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a test case would be nice for checking this behavior, rest of comments are just pr feedback/suggestions

👍

EDIT: also check the CI failure https://travis-ci.org/gin-gonic/gin/jobs/291989136#L555

@codecov
Copy link

codecov bot commented Oct 24, 2017

Codecov Report

Merging #1138 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1138      +/-   ##
==========================================
+ Coverage   98.41%   98.41%   +<.01%     
==========================================
  Files          34       34              
  Lines        1762     1772      +10     
==========================================
+ Hits         1734     1744      +10     
  Misses         23       23              
  Partials        5        5
Impacted Files Coverage Δ
binding/form_mapping.go 98.42% <100%> (+0.13%) ⬆️

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 814ac94...a41721a. Read the comment docs.

@thinkerou
Copy link
Member Author

@javierprovecho I will prefect the pull request include test and comment, thanks!

@appleboy
Copy link
Member

@thinkerou Please fix the conflicts.

@thinkerou
Copy link
Member Author

@appleboy fixed.

appleboy
appleboy previously approved these changes Apr 17, 2018
@appleboy appleboy merged commit 41f951e into gin-gonic:master Apr 25, 2018
tonyhhyip pushed a commit to ysitd-cloud/gin that referenced this pull request Apr 28, 2018
* support default value for form

* fix bug for nil interface

* use SplitN and optimization code

* add test case

* add test cases for form(own default value)

* fix invalid code

* fix code indent

* assert order

(cherry picked from commit 41f951e)
@thinkerou thinkerou deleted the default_form branch May 2, 2018 02:26
@@ -38,8 +48,13 @@ func mapForm(ptr interface{}, form map[string][]string) error {
}
}
inputValue, exists := form[inputFieldName]

if !exists {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if inputValue is empty array(inputField exist and its ""), default value not take effect

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you post one example? thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants