-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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.attachment append a parameter: options from contentDisposition #1240
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1240 +/- ##
======================================
Coverage 100% 100%
======================================
Files 5 5
Lines 391 391
======================================
Hits 391 391
Continue to review full report at Codecov.
|
can you add a test case for this to avoid later changes modified this feature. |
@dead-horse thanks, already done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also update docs at https://github.com/koajs/koa/blob/master/docs/api/response.md#responseattachmentfilename
Otherwise LGTM
test/response/attachment.js
Outdated
@@ -48,3 +48,138 @@ describe('ctx.attachment([filename])', () => { | |||
}); | |||
}); | |||
}); | |||
|
|||
// reference [content-disposition test case](https://github.com/jshttp/content-disposition/blob/master/test/test.js) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't make this comment md-style
Ok, I will do it two days later. |
@fl0w already done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
response.attachment require content-disposition, and content-disposition supports option parameter to make some settings, so i hope attachment can also support it.
thanks !