-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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 convenience method for HTTP OPTIONS #2541
Add convenience method for HTTP OPTIONS #2541
Conversation
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.
+1, this makes sense to support along with the other HTTP methods. I'm surprised we don't already.
Added a few comments, but overall looks good to me
package.json
Outdated
@@ -7,7 +7,7 @@ | |||
"util", | |||
"utility" | |||
], | |||
"version": "2.79.1", | |||
"version": "2.80.0", |
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.
yup, pull this out. We manage versions independently from PRs
|
||
tape('options(string, function)', function (t) { | ||
request.options(s.url + '/options', function (e, r) { | ||
t.equal(r.headers['x-original-method'], 'OPTIONS') |
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.
- assert that there was no error
- assert the response code
url: s.url + '/options', | ||
headers: { foo: 'bar' } | ||
}, function (e, r) { | ||
t.equal(r.headers['x-original-method'], 'OPTIONS') |
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.
- assert that there was no error
- assert the response code
Awesome, thanks for your feedback @FredKSchott. Hopefully I'll be able to make the changes this evening. |
@FredKSchott I've made the changes and CI looks happy. Let me know if there's anything else you need! |
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 Will merge tomorrow if there's no other reviews/comments
Nice one. Thank you 😊 |
Hey @FredKSchott, is everything good to go with this PR? Thanks! |
Hey @FredKSchott, do you know when this might be merged? It would be super useful for my testing. Cheers! |
@jamesseanwright thanks for pinging, this kept getting lost in my notifications. Merging now |
Awesome, thank you @FredKSchott! :) |
Hey guys,
I'm currently working on a feature that involves a HTTP OPTIONS request. I really appreciate the convenience methods as they are easy to stub with my library of choice (Sinon).
I noticed that there is not an equivalent for HTTP OPTIONS, so I've added one. There may be a reason why this hasn't been exposed already, but implementing it didn't impact any of the existing tests, plus searching the existing PRs and issues didn't suggest anything.
My PR also has new version number (i.e. new minor revision), but this is merely a suggestion; I'm sure you'll have your own processes surrounding this.
Let me know if you have any questions or thoughts.
Thanks,
James