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

Serve easily dynamic files with DataFromReader context method #1304

Merged
merged 4 commits into from May 12, 2018

Conversation

jclebreton
Copy link
Contributor

@jclebreton jclebreton commented Apr 2, 2018

This pull request adds a new method called DataFromReader to provides an easy way to serve dynamic files.

@codecov
Copy link

codecov bot commented Apr 2, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1304      +/-   ##
==========================================
+ Coverage   98.19%   98.21%   +0.01%     
==========================================
  Files          34       35       +1     
  Lines        1826     1846      +20     
==========================================
+ Hits         1793     1813      +20     
  Misses         26       26              
  Partials        7        7
Impacted Files Coverage Δ
render/render.go 100% <ø> (ø) ⬆️
context.go 96.13% <100%> (+0.07%) ⬆️
render/reader.go 100% <100%> (ø)

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 5636afe...e20a756. Read the comment docs.

@jclebreton jclebreton changed the title Add DataFromReader context method Serve easily dynamic files with DataFromReader context method Apr 2, 2018
@appleboy
Copy link
Member

appleboy commented May 1, 2018

@thinkerou Could you also help to review this PR?

@thinkerou
Copy link
Member

thinkerou commented May 1, 2018

@appleboy @jclebreton Except for comments, LGTM to me. Thanks!

@@ -834,6 +834,32 @@ func main() {
}
```

### Serving data from reader
Copy link
Member

Choose a reason for hiding this comment

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

Please add content link in README, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @thinkerou I'm not sure to understand the target of the asked link. This page ?

Copy link
Member

@thinkerou thinkerou May 8, 2018

Choose a reason for hiding this comment

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

You should add [Serving data from reader](#serving-data-from-reader) after the line:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks

render/reader.go Outdated
// Render (Reader) writes data with custom ContentType and headers.
func (r Reader) Render(w http.ResponseWriter) (err error) {
r.WriteContentType(w)
r.Headers["Content-Length"] = fmt.Sprintf("%d", r.ContentLength)
Copy link
Member

Choose a reason for hiding this comment

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

Why not use strconv.Itoa(r.ContentLength)? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. It's better to use strconv library but to convert a int64 variable to a string strconv.FormatInt(r.ContentLength, 10) works better ;)

Copy link
Member

Choose a reason for hiding this comment

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

Good!

@jclebreton jclebreton force-pushed the data_from_reader branch 2 times, most recently from 68b44cf to 157ffdb Compare May 8, 2018 10:24
"testing"

"strings"
Copy link
Member

Choose a reason for hiding this comment

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

remove the extra empty line between internal package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done ;-)

@appleboy
Copy link
Member

appleboy commented May 9, 2018

need @thinkerou approvl.

@thinkerou
Copy link
Member

LGTM

func (r Reader) writeHeaders(w http.ResponseWriter, headers map[string]string) {
header := w.Header()
for k, v := range headers {
if val := header[k]; len(val) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Actually, I prefer to using val == "" for judging empty string.

Copy link
Member

Choose a reason for hiding this comment

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

@jclebreton Please update and I will merge this.

@jclebreton
Copy link
Contributor Author

Thanks for review and merge ;-)

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.

None yet

3 participants