Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

allow overriding the http request scheme #75

Merged
merged 3 commits into from
Dec 8, 2016
Merged

Conversation

amsoell
Copy link

@amsoell amsoell commented Nov 29, 2016

Added a simple way to override the default "scheme" property of the http request. When using apac in a desktop application such as Electron, all http requests are attempted with the 'file://" scheme by default—this provides a way to force the requests to go through via http or https

@dmcquay
Copy link
Owner

dmcquay commented Dec 1, 2016

Thanks for your contribution. This looks great except that it is lacking tests. Please update operation-helper.specs.js to cover this case.

@dmcquay
Copy link
Owner

dmcquay commented Dec 5, 2016

Awesome, thanks for following through. Reviewing...

@dmcquay
Copy link
Owner

dmcquay commented Dec 5, 2016

You are testing that the option is passed in and set on the operation helper, but you are not testing that the scheme is passed into the http options. At minimum, I suggest you add the scheme option to the existing happy path test here: https://github.com/MettleUp/node-apac/blob/010f78f0b410e845c547ac5b0aea9cd2077ed7b0/lib/operation-helper.specs.js#L161 and assert that it was passed into the http requests around here: https://github.com/MettleUp/node-apac/blob/010f78f0b410e845c547ac5b0aea9cd2077ed7b0/lib/operation-helper.specs.js#L191

@amsoell
Copy link
Author

amsoell commented Dec 8, 2016

Sorry, my JS testing chops are a little dusty— let me know if there's anything else you suggest here.

@dmcquay dmcquay merged commit 008e3a0 into dmcquay:master Dec 8, 2016
@dmcquay
Copy link
Owner

dmcquay commented Dec 8, 2016

Looks great! Thanks @amsoell.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants