-
Notifications
You must be signed in to change notification settings - Fork 40
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
Replace request with got #169
base: master
Are you sure you want to change the base?
Conversation
This replaces the deprecated requests library with the got library. Unfortunately got 14.x is required for permitting followRedirect to be a function, and got 14.x requires Node 20.
@@ -242,7 +252,7 @@ describe('proxy', function() { | |||
[methodName]('/https://example.com/blah') | |||
.expect(200) | |||
.expect(function() { | |||
expect(fakeRequest.calls.argsFor(0)[0].proxy).toBe('http://proxy/'); | |||
expect(fakeRequest.calls.argsFor(0)[0].agent.http.proxy).toEqual(expectedProxy); |
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.
Only changing .proxy
into .agent.http.proxy
made the tests fail with
Expected http://proxy/ to be 'http://proxy/'
And I have no idea why.
if (response.statusCode >= 300) { | ||
return response; | ||
} else { | ||
return response.body.files[Object.keys(response.body.files)[0]].content; // find the contents of the first file in the gist |
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.
Got seems pretty compatible with request, many options have the same names.
However, I don't think this is tested by the test suite, and I have no idea if it is correct with got.
(response, _retryWithMergedOptions) => { | ||
if (response.statusCode === 201) { | ||
console.log('Created ID ' + response.body.id + ' using Gist service'); | ||
return response.body.id; |
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.
Doubt this is tested by the test suite too, and I have no idea if body has an id attribute with got.
After running |
This replaces the deprecated
requests library with the got library.
Unfortunately got 14.x is required
for permitting followRedirect to
be a function, and got 14.x requires
Node 20.