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 PureJSON renderer #694

Merged
merged 19 commits into from Aug 20, 2018
Merged

Add PureJSON renderer #694

merged 19 commits into from Aug 20, 2018

Conversation

ffigiel
Copy link
Contributor

@ffigiel ffigiel commented Aug 31, 2016

Closes #693

@ffigiel
Copy link
Contributor Author

ffigiel commented Aug 31, 2016

Looks like it's not compiling 😆
The error is:

@PureJSON $ ~/gin > go test
# _/home/filip/gin
./context.go:427: undefined: render.PureJSON
FAIL    _/home/filip/gin [build failed]

could someone help me out? The code looks good to me.

Edit: error logs from travis were more helpful, will fix

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 93.971% when pulling bcf1052 on megapctr:PureJSON into f931d1e on gin-gonic:develop.

@cristaloleg
Copy link

@megapctr Maybe you should add PureJSON interface{} here: https://github.com/rikuayanokozy/gin/blob/743cdd1a39fefa28e4e6f86d81a02b82512b433d/context.go#L499

@ffigiel
Copy link
Contributor Author

ffigiel commented Sep 5, 2016

@cristaloleg the problem is that this feature is available in go 1.7 only. I don't know how to handle that for earlier versions. Any tips?

@ffigiel
Copy link
Contributor Author

ffigiel commented Sep 5, 2016

I'm referring to encoder.SetEscapeHTML specifically

@robvdl
Copy link

robvdl commented Sep 6, 2016

Sorry for being a bit blunt, but the way you could be dealing with the Go 1.7 issue is by first releasing version 1.0 of Gin, it's about time to put a line in the sand and doing a 1.0 release (Gin to me seems a bit "stuck" to me as 1.0 never comes out). Then any Gin releases after 1.0 making them for Go 1.7 only. Then users of Go 1.6 and below can still use the 1.0 release if they want.

@ffigiel
Copy link
Contributor Author

ffigiel commented Sep 6, 2016

I see. It's ok for me to wait until gin is ready for the next step. Besides, this PR isn't any crucial feature at all. Sending a json works fine either way. With PureJSON you cut a few bytes off from the response size, which may be helpful in some rare cases

@appleboy
Copy link
Member

@megapctr Could you fix the conflicts?

@appleboy
Copy link
Member

@megapctr conflicts.

t,
w.Body.String(),
"{\"foo\":\"bar\",\"html\":\"\\u003cb\\u003e\"}\n")
assert.Equal(t, w.Header().Get("Content-Type"), "application/json; charset=utf-8")
Copy link
Member

Choose a reason for hiding this comment

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

should be assert.Equal(t, expect, actual)

@thinkerou
Copy link
Member

@megapctr can you help to add example to readme? thanks!

@thinkerou
Copy link
Member

thinkerou commented Aug 17, 2018

compile error: render.PureJSON does not implement render.Render (missing WriteContentType method) @megapctr

@ffigiel
Copy link
Contributor Author

ffigiel commented Aug 17, 2018

I have trouble running tests locally so I'll rely on travis

@ffigiel
Copy link
Contributor Author

ffigiel commented Aug 17, 2018

@thinkerou you mean the main readme? I think it would be identical to the existing example, except it would use c.PureJSON instead of c.JSON.

What should I do to support go 1.6?

@thinkerou
Copy link
Member

@megapctr add https://github.com/gin-gonic/gin#jsonp after. Now still support go1.6.

@appleboy appleboy added this to the 1.4 milestone Aug 17, 2018
@ffigiel
Copy link
Contributor Author

ffigiel commented Aug 17, 2018

@appleboy What should I do about go 1.6?

@appleboy
Copy link
Member

https://golang.org/doc/go1.7

maybe we should create new file and add build tag in top of file

// +build go1.7

@ffigiel
Copy link
Contributor Author

ffigiel commented Aug 18, 2018

Travis hates me today 😢 Apart from that is everything looking ok?

context_17.go Outdated
@@ -0,0 +1,17 @@
// Copyright 2014 Manu Martinez-Almeida. All rights reserved.
// Use of this source code is governed by a MIT style
// license that can be found in the LICENSE file.
Copy link
Member

Choose a reason for hiding this comment

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

// Copyright 2018 Gin Core Team.  All rights reserved.
// Use of this source code is governed by a MIT style
// license that can be found in the LICENSE file.

// Copyright 2014 Manu Martinez-Almeida. All rights reserved.
// Use of this source code is governed by a MIT style
// license that can be found in the LICENSE file.

Copy link
Member

Choose a reason for hiding this comment

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

// Copyright 2018 Gin Core Team.  All rights reserved.
// Use of this source code is governed by a MIT style
// license that can be found in the LICENSE file.

@@ -0,0 +1,28 @@
// Copyright 2014 Manu Martinez-Almeida. All rights reserved.
// Use of this source code is governed by a MIT style
// license that can be found in the LICENSE file.
Copy link
Member

Choose a reason for hiding this comment

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

// Copyright 2018 Gin Core Team.  All rights reserved.
// Use of this source code is governed by a MIT style
// license that can be found in the LICENSE file.

// Copyright 2014 Manu Martinez-Almeida. All rights reserved.
// Use of this source code is governed by a MIT style
// license that can be found in the LICENSE file.

Copy link
Member

Choose a reason for hiding this comment

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

// Copyright 2018 Gin Core Team.  All rights reserved.
// Use of this source code is governed by a MIT style
// license that can be found in the LICENSE file.

@appleboy
Copy link
Member

@thinkerou I don't know why make vet only can't work on go latest version. maybe we can ignore the error from Travis.

appleboy
appleboy previously approved these changes Aug 19, 2018
@thinkerou
Copy link
Member

Oh....@appleboy Makefile not update lead to the error, because the pull request commit to develop branch. so recommit to master branch or change it(how change?).

thinkerou
thinkerou previously approved these changes Aug 20, 2018
func TestContextRenderPureJSON(t *testing.T) {
w := httptest.NewRecorder()
c, _ := CreateTestContext(w)
c.PureJSON(201, H{"foo": "bar", "html": "<b>"})
Copy link
Member

@thinkerou thinkerou Aug 18, 2018

Choose a reason for hiding this comment

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

please change 201 to http status code

context_test.go Outdated
func TestContextRenderJSON(t *testing.T) {
w := httptest.NewRecorder()
c, _ := CreateTestContext(w)

c.JSON(201, H{"foo": "bar"})
c.JSON(201, H{"foo": "bar", "html": "<b>"})

assert.Equal(t, 201, w.Code)
Copy link
Member

@thinkerou thinkerou Aug 18, 2018

Choose a reason for hiding this comment

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

please change 201 to http status code

@@ -0,0 +1,28 @@
// Copyright 2014 Manu Martinez-Almeida. All rights reserved.
// Use of this source code is governed by a MIT style
// license that can be found in the LICENSE file.
Copy link
Member

@thinkerou thinkerou Aug 18, 2018

Choose a reason for hiding this comment

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

please change to

// Copyright 2018 Gin Core Team.  All rights reserved.
// Use of this source code is governed by a MIT style
// license that can be found in the LICENSE file.

@@ -0,0 +1,26 @@
// Copyright 2014 Manu Martinez-Almeida. All rights reserved.
// Use of this source code is governed by a MIT style
// license that can be found in the LICENSE file.
Copy link
Member

Choose a reason for hiding this comment

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

please change to

// Copyright 2018 Gin Core Team. All rights reserved.
// Use of this source code is governed by a MIT style
// license that can be found in the LICENSE file.

c, _ := CreateTestContext(w)
c.PureJSON(201, H{"foo": "bar", "html": "<b>"})
assert.Equal(t, 201, w.Code)
assert.Equal(t, "{\"foo\":\"bar\",\"html\":\"<b>\"}\n", w.Body.String())
Copy link
Member

Choose a reason for hiding this comment

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

too

context_test.go Outdated
assert.Equal(
t,
"{\"foo\":\"bar\",\"html\":\"\\u003cb\\u003e\"}",
w.Body.String())
Copy link
Member

Choose a reason for hiding this comment

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

suggest to use one line

assert.Equal(
t,
"{\"foo\":\"bar\",\"html\":\"\\u003cb\\u003e\"}\n",
w.Body.String())
Copy link
Member

Choose a reason for hiding this comment

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

suggest to use one line

@thinkerou
Copy link
Member

@megapctr thanks a lot, firstly, can you fix some code according to my review result, secondly, please recommit the pull request to master branch, because it will error which pointed above. thanks again.

@ffigiel ffigiel changed the base branch from develop to master August 20, 2018 06:52
@ffigiel ffigiel dismissed stale reviews from thinkerou and appleboy via b1cf3bf August 20, 2018 06:55
Copy link
Member

@thinkerou thinkerou left a comment

Choose a reason for hiding this comment

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

LGTM and thanks a lot!

Copy link
Member

@appleboy appleboy left a comment

Choose a reason for hiding this comment

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

LGTM

@appleboy appleboy merged commit c6110f9 into gin-gonic:master Aug 20, 2018
@bjm88
Copy link

bjm88 commented Dec 17, 2018

Hi, specifically for context.PureJSON - in latest gin version dont see this as option...did I miss something or this never got into release from master? How to render json without escaping?

@winjeg
Copy link

winjeg commented Jan 22, 2019

Hi, specifically for context.PureJSON - in latest gin version dont see this as option...did I miss something or this never got into release from master? How to render json without escaping?

The latest release is on 14 Aug 2018.

v1.3.0
@javierprovecho javierprovecho released this on 14 Aug 2018 · 99 commits to master since this release

The feature is merged on 20 Aug 2018

appleboy merged commit c6110f9 into gin-gonic:master  on 20 Aug 2018

So you don't see the c.PureJSON() Method.
It took two years for those guys to solve this problem, but not yet released!

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

9 participants