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 ability response body and response content type #18

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

TheSmartnik
Copy link

Hi! Thank you for the clammit!

I thought that it could be useful to specify custom response body and content-type. For example, if backend has a convention for errors with specified body and content-type frontend wouldn't have to handle 418 as a special case.

I'm not familiar wit Go, so apologies for any apparent mistake, would love to fix them

@TheSmartnik
Copy link
Author

@vjt could you take a look, please?

Copy link
Contributor

@vjt vjt left a comment

Choose a reason for hiding this comment

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

This looks reasonable and good, thank you.

Just a small change requested and I'll merge.

VirusStatusCode: 418,
ContentMemoryThreshold: 1024 * 1024,
VirusResponseBody: "",
VirusResponseContentType: "text/plain; charset=utf-8",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please set a default here, and...


if len(c.VirusResponseBody) > 0 {
w.Write([]byte(c.VirusResponseBody))
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

...remove this if from here, given that there will always be a response body defined.

@vjt
Copy link
Contributor

vjt commented Mar 23, 2020

ps. sorry for the delay...

w.Write([]byte(c.VirusResponseBody))
} else {
w.Write([]byte(fmt.Sprintf("File %s has a virus!", filename)))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes we are losing the filename in the response - however it's not a big loss :-)

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