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

add RenderWith #773

Closed
wants to merge 2 commits into from
Closed

add RenderWith #773

wants to merge 2 commits into from

Conversation

mei-rune
Copy link

RenderWith will is not panic while render is fail.

RenderWith will is not panic while render is fail.
@codecov-io
Copy link

codecov-io commented Dec 23, 2016

Codecov Report

Merging #773 into develop will decrease coverage by 2.99%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           develop     #773    +/-   ##
=========================================
- Coverage    96.17%   93.18%    -3%     
=========================================
  Files           16       15     -1     
  Lines         1413     1291   -122     
=========================================
- Hits          1359     1203   -156     
- Misses          43       76    +33     
- Partials        11       12     +1
Impacted Files Coverage Δ
context.go 86.69% <100%> (-9.58%) ⬇️
debug.go 58.82% <0%> (-41.18%) ⬇️
errors.go 97.18% <0%> (-2.82%) ⬇️
gin.go 87.91% <0%> (-2.73%) ⬇️
fs.go 100% <0%> (ø) ⬆️
logger.go 100% <0%> (ø) ⬆️
deprecated.go 0% <0%> (ø) ⬆️
tree.go 100% <0%> (ø) ⬆️
test_helpers.go

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 6613cdb...56f1a06. Read the comment docs.

@appleboy
Copy link
Member

Please provide testing code.

@robvdl
Copy link

robvdl commented Dec 23, 2016

I know this will break backwards compatibility so isn't likely to get renamed, but shouldn't RenderWith technicall be called Render as it returns an error, and Render then called MustRender (since it panics) and that seems to be the naming convention in Go for methods that panic.

But this is the next best thing, thanks, I really needed a render method that doesn't panic.

I also had an issue that the Instance method panics rather than return an error, which was a bit annoying when I was building a custom renderer a while back, issue #678 described that.

add unit test of Context.RenderWith()
@mei-rune
Copy link
Author

I add test code

@robvdl
Copy link

robvdl commented Dec 23, 2016

I know I am not one of the Gin contributors so I don't really have say in this, so merge it if you want, but...

I still stand by the fact that these methods are named incorrectly, Render should become MustRender since it's panics.

RenderWith is a terrible name as it indicates you are rendering with something being passed to it, possibly a context or something. I personally would actually call RenderWith just Render, and call Render MustRender.

I would do so in v2, breaking backwards compatibility for the greater good.

@appleboy
Copy link
Member

@javierprovecho @tboerger What do you guys think about naming?

@themartorana
Copy link

Alternatively, #952 changes Context.Render (and Context.JSON) from a panic to returning an error. It's non-breaking, although recovery deferments will become useless. Arguments for never panic-ing are myriad - especially considering broken pipes are rarely the fault of the server.

@appleboy appleboy changed the base branch from master to develop June 28, 2017 00:49
@appleboy appleboy added this to the 1.2 milestone Jun 28, 2017
@javierprovecho javierprovecho modified the milestones: 1.2, 1.3 Jul 2, 2017
@javierprovecho javierprovecho modified the milestones: 1.3, 1.4 Aug 14, 2018
@thinkerou
Copy link
Member

@runner-mei please re-commit the pull request to master branch, thanks!

@thinkerou
Copy link
Member

RenderWith is a terrible nam

@robvdl ShouldRender, how?

@mei-rune
Copy link
Author

TryRender
or
ShouldRender

@thinkerou
Copy link
Member

ref: ShouldBind, I like ShouldRender.

@robvdl
Copy link

robvdl commented Oct 26, 2018

@thinkerou because "render with" sounds like you are rendering with ... something ... like a context, the name name does not describe what it really does which is that it does a render but does panic on error.

I don't think you should use ShouldRender or ShouldBind either because the normal naming convention in Go is to use MustRender when something panics, and Render it if doesn't. It seems that pre-pending Must is the usual convention used.

I personally think it's much better to use the naming convention the Go community is already using rather than use something different.

@mei-rune mei-rune closed this Nov 5, 2018
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

7 participants