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 a requestUrl property to the response object #205

Merged
merged 5 commits into from
Jun 25, 2016
Merged

Add a requestUrl property to the response object #205

merged 5 commits into from
Jun 25, 2016

Conversation

luanmuniz
Copy link
Contributor

@luanmuniz luanmuniz commented Jun 21, 2016

Update from #188

Needed a new PR because the previous repository doesn't exist anymore and because of this is impossible to update that one. (Sorry about that)

@sindresorhus Is this what you meant in #188?
By the way: My writing in english os not very good so please take a look if it doesn't have any gramatical mistakes

cc @floatdrop

@@ -127,7 +127,7 @@ Option accepts `function` with `retry` and `error` arguments. Function must retu

###### followRedirect

Type: `boolean`
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't change this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a trailing space

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@luanmuniz this will remove line breake before Default. You can see it in md preview.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just added a <br> like in the other lines

@sindresorhus sindresorhus changed the title Add requestUrl to response Add a requestUrl property to the response object Jun 22, 2016
@sindresorhus
Copy link
Owner

@floatdrop You good with this?

@@ -41,6 +41,10 @@ test('empty response', async t => {
t.is((await got(`${s.url}/empty`)).body, '');
});

test('requestUrl response', async t => {
t.is((await got(s.url)).requestUrl, `${s.url}/`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test with object {host: s.host, port: s.port} as argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its done. Can you check if what i did is what you ask for?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@luanmuniz yup.

@@ -106,6 +106,7 @@ function asPromise(opts) {
const limitStatusCode = opts.followRedirect ? 299 : 399;

res.body = data;
res.requestUrl = opts.href || urlLib.resolve(urlLib.format(opts), opts.path);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't res.requestUrl = urlLib.format(opts); suffice?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, when sending an object we don't have opts.href and urlLib.format(opts) don't return the opts.path in it. I just put the opts.href because if it exist we don't need to send to the functions. But it is removable.

@sindresorhus
Copy link
Owner

LGTM

@floatdrop
Copy link
Contributor

👍

@floatdrop floatdrop merged commit 1409f82 into sindresorhus:master Jun 25, 2016
floatdrop pushed a commit that referenced this pull request Nov 1, 2016
* Add requestUrl to response, update documentation and tests

* Add test when redirect

* Fix return description

* Add tests with params and fix test

* Add linebreak
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

3 participants