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

response: clarify Content-Disposition, close file in Stream example #351

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

StalkR
Copy link
Contributor

@StalkR StalkR commented Oct 2, 2024

Hello,

I just discovered Echo while reading Stripe's payments Go demo, I'm going through the docs and so far I'm a big fan of the minimalistic approach! I hate bloat and love minimalist things (e.g. also just found PicoCSS for web design). Good stuff, can't wait to start coding with it, and thank you for sharing it.

Anyhow, this PR proposes 2 minor changes:

  1. When reading the Response doc, it wasn't clear to me what were the differences between Context#File, Context#Attachment and Context#Inline. Reading the source made it clearer: the former 2 are referring to sending Content-Disposition with attachment and inline, which makes total sense. Therefore, I propose adding Content-Disposition to make this unambiguous.

  2. Adding missing defer f.Close() in the Context#Stream example. I also checked, it takes an io.Reader not an io.ReadCloser so it couldn't delegate the close to Context#Stream, which could have worked otherwise!

It wasn't clear to me what were the differences between `Context#File`,
`Context#Attachment` and `Context#Inline` - reading the source made it
clearer: the former 2 are referring to sending `Content-Disposition`
with `attachment` and `inline`, which makes sense.

Adding missing `defer f.Close()` in the `Context#Stream` example. I also
checked, it takes an `io.Reader` not an `io.ReadCloser` so it couldn't
delegate the close to `Context#Stream`.
@aldas aldas merged commit 34cd13e into labstack:master Oct 3, 2024
1 check passed
@StalkR StalkR deleted the response branch October 3, 2024 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants