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 convenience method for HTTP OPTIONS #2541

Merged
merged 3 commits into from Apr 17, 2017
Merged

Add convenience method for HTTP OPTIONS #2541

merged 3 commits into from Apr 17, 2017

Conversation

jamesseanwright
Copy link
Contributor

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

Copy link
Contributor

@FredKSchott FredKSchott left a 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",
Copy link
Contributor

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')
Copy link
Contributor

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')
Copy link
Contributor

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

@jamesseanwright
Copy link
Contributor Author

Awesome, thanks for your feedback @FredKSchott. Hopefully I'll be able to make the changes this evening.

@jamesseanwright
Copy link
Contributor Author

@FredKSchott I've made the changes and CI looks happy. Let me know if there's anything else you need!

Copy link
Contributor

@FredKSchott FredKSchott left a 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

@jamesseanwright
Copy link
Contributor Author

Nice one. Thank you 😊

@jamesseanwright
Copy link
Contributor Author

Hey @FredKSchott, is everything good to go with this PR? Thanks!

@jamesseanwright
Copy link
Contributor Author

Hey @FredKSchott, do you know when this might be merged? It would be super useful for my testing. Cheers!

@FredKSchott
Copy link
Contributor

@jamesseanwright thanks for pinging, this kept getting lost in my notifications. Merging now

@FredKSchott FredKSchott merged commit a765593 into request:master Apr 17, 2017
@jamesseanwright
Copy link
Contributor Author

Awesome, thank you @FredKSchott! :)

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.

None yet

2 participants