ShouldBind counterparts for Bind methods

[sudo-suhas]

Issue Description

Currently, the functions ctx.Bind, ctx.BindJSON, ctx.BindQuery all use ctx.MustBindWith.
In my opinion, the current implementation of ctx.MustBindWith has the following drawbacks. Note that this has been discussed before and I am including it here to avoid needing to crawl multiple issues, threads:

• Lack of control
  • It sets the HTTP status code as 400 and developer cannot set any other HTTP status code(ex: 422, http.StatusUnprocessableEntity)
  • It aborts the handler chain. Developer should be able to decide whether the resulting error is worth aborting or can be reconciled.
  • Cannot use repeated Bind as discussed in Bind JSON and Query? #811. Developer might want to bind query params and JSON body separately.
• Possibly incorrect Content-type header
  • It sets the Content-type as text/plain
  • Neither ctx.JSON nor manually trying to set the content type header help. Instead we see this warning:

    [GIN-debug] [WARNING] Headers were already written. Wanted to override status code 400 with 422
    

Proposal

For backward compatibility, we cannot change the behavior of existing bind methods. So this PR adds ShouldBind counterparts - ctx.ShouldBind, ctx.ShouldBindJSON, ctx.ShouldBindQuery.

Note that I have not yet updated the readme file and will do so depending on the outcome of the discussion here. I suggest that we switch over all examples on readme to use the Should equivalents.
All examples on readme have been updated to use ShouldBind methods. Also added section explaining difference between Bind and ShouldBind methods.
For example, this isn't going to work if there is a bind error:

func startPage(c *gin.Context) {
    var person Person
    if c.BindQuery(&person) == nil {
        log.Println("====== Only Bind By Query String ======")
        log.Println(person.Name)
        log.Println(person.Address)
    }
    c.String(200, "Success") // Can cause `[GIN-debug] [WARNING] Headers were already written.`
}

I am new to Go 🙇 . I have pretty much duplicated the existing tests for bind methods with Should equivalents. Please let me know if anything needs to be changed.

Related issues - #629, #633, #662, #782, #811
Related PR - #636, #661
Closes #840

[codecov]

Codecov Report

Merging #1047 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1047      +/-   ##
==========================================
+ Coverage   96.67%   96.68%   +0.01%     
==========================================
  Files          16       16              
  Lines        1714     1721       +7     
==========================================
+ Hits         1657     1664       +7     
  Misses         49       49              
  Partials        8        8

Impacted Files	Coverage Δ
context.go	96.64% <100%> (+0.06%)	⬆️

---

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 b8b6831...e8d927a. Read the comment docs.

[sudo-suhas]

@appleboy Shall I update the readme to use ShouldBind methods? Seems like this is causing considerable confusion for users.

[appleboy]

@sudo-suhas OK, Welcome

[pkopac]

@sudo-suhas, @appleboy: thanks for introducing the ShouldBindWith. The incorrect Content-type (as text/plain) was breaking my API, but this is a nice workaround. Would be handy if the BindJSON method actually set Content-type based on the request, as it looks it is meant to from the code.

[sudo-suhas]

Rebased. Could we please move forward with this? I can do any changes if needed.

[appleboy]

@javierprovecho Please help to review this PR. Waiting for your feedbacks.

[mymtw]

maybe rename/refactor your methods ShouldBindJSON, ShouldBindQuery into bindJSON, bindQuery instead of adding yet new binders?
Also there are already exists ShouldBindWith which can be renamed into BindWith
because new methods makes confusion, redundant code IMHO

[sudo-suhas]

As I mentioned earlier:

For backward compatibility, we cannot change the behavior of existing bind methods. So this PR adds ShouldBind counterparts - ctx.ShouldBind, ctx.ShouldBindJSON, ctx.ShouldBindQuery.

Additionally, this PR also adds relevant documentation to explain and clarify the differences between Bind and Shouldbind methods.

[mymtw]

I'm against backward compatibility, there are exist previous version of framework, this can be added in 1.3
as mentioned below, let's maintainers decide

[sudo-suhas]

I think that is for the repo maintainers to decide. I am more than willing to do the requested changes.

[sudo-suhas]

ping @javierprovecho @appleboy

[sudo-suhas]

Hey @javierprovecho, @appleboy, it has been over 2 months since I first initiated this pull request. Is there anything at all, that I can do, so that we move forward with this? Any communication, even if it is to reject this pull requests, would be welcome. While I am aware that you would be working on this in your free time, IMHO, lack of communication can really make it harder for contributors to make repeat pull requests.

[javierprovecho]

@sudo-suhas I agree with you, it is my fault for leaving somehow unattended the repo. I'm now most convinced that it is responsibility of the developer to lock version/commit of the framework. The project have been for more than two minor versions with an indication in the readme about how to use vendor. I apologize about the delay.

[javierprovecho]

@sudo-suhas thanks for your pr!

[sudo-suhas]

@javierprovecho Thank you for accepting the PR 😄. Could you explain the context for your comment:

I'm now most convinced that it is responsibility of the developer to lock version/commit of the framework. The project have been for more than two minor versions with an indication in the readme about how to use vendor.

[javierprovecho]

@sudo-suhas I've been holding many pr that break stability on api level or behavior level (ex: #1061) because when gin originally was published, there was no vendor tool popular enough for locking into a specific version.

The project have been for more than two minor versions with an indication in the readme about how to use vendor.

As said, https://github.com/gin-gonic/gin#use-a-vendor-tool-like-govendor it has been present for some months since now and vendoring is a common practice in the golang community since ~ 1.6

EDIT: of course, this doesn't mean the project goes crazy now, just a bit (lot) less conservative 😉 with its corresponding warnings in the changelog and common sense of course

[sudo-suhas]

Yes, I also use vendoring. I also made a PR for gin to use dep which is slated to be the official vendoring tool for Go(sometime in the near future). I was a little confused because this PR did not break backward compatibility and only added new functionality.

[javierprovecho]

@sudo-suhas about that, another concern I had with your pr was that the API would not scale properly in the future with long named funcs, but that will have an easier solution that I'm planning

EDIT: i'll need to check dep, thats a pending feature 👍