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

Slice don't bind the route parameter #980

Closed
wants to merge 1 commit into from

Conversation

tap1ra
Copy link

@tap1ra tap1ra commented Aug 2, 2017

hi.
Support of Bind to Slice of Struct which was possible before this PR (#973) was supported.
Because BindData is always invoked, Struct was mandatory.

As another solution, I studied to make the route parameter Bind to each element Struct of Slice, but I stopped because I seemed unnatural.

Please comment if Bind does not assume Slice of Struct.
I will withdraw this PR and take action on the user side.

@codecov
Copy link

codecov bot commented Aug 2, 2017

Codecov Report

Merging #980 into master will decrease coverage by 0.11%.
The diff coverage is 40%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #980      +/-   ##
==========================================
- Coverage   76.67%   76.55%   -0.12%     
==========================================
  Files          27       27              
  Lines        2229     2231       +2     
==========================================
- Hits         1709     1708       -1     
- Misses        398      400       +2     
- Partials      122      123       +1
Impacted Files Coverage Δ
bind.go 80.29% <40%> (-1.3%) ⬇️

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 f8c3008...1c3bd80. Read the comment docs.

@pyama86
Copy link

pyama86 commented Aug 2, 2017

fix: #891

@vishr vishr closed this in b42edd7 Aug 3, 2017
@vishr
Copy link
Member

vishr commented Aug 3, 2017

@tap1ra thanks for your contribution, I took your changes and got it merged with mine.

@vishr vishr self-requested a review August 3, 2017 06:17
@vishr vishr assigned vishr and tap1ra Aug 3, 2017
@vishr vishr added the bug label Aug 3, 2017
@tap1ra
Copy link
Author

tap1ra commented Aug 3, 2017

Thanks!!!

@tap1ra
Copy link
Author

tap1ra commented Aug 3, 2017

@vishr

echo/bind.go

Lines 42 to 47 in b42edd7

if err = b.bindPathData(i, c); err != nil {
return NewHTTPError(http.StatusBadRequest, err.Error())
}
if err = b.bindPathData(i, c); err != nil {
return NewHTTPError(http.StatusBadRequest, err.Error())
}

What kind of intention is it?
(paste mistake?)

@vishr
Copy link
Member

vishr commented Aug 3, 2017

Yes, it was a copy paste mistake, I have fixed it.

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

Successfully merging this pull request may close these issues.

3 participants