Skip to content

Conversation

@glassmonkey
Copy link

@glassmonkey glassmonkey commented May 6, 2022

This is almost a copy of this Pull Request #321.

The final url is extract from XMLHttpRequest.responseURL(on browsers) or HttpClientResponse.redirects.last.location(on vm).
Also added tests for vm.

Additional work includes support for Null Safety and tests created from streams.

I've create new patch, since the original author @yizhepku mention the following in #293.

Alas, I've long forgotten about this and have deleted my repo. The content of the patch is still viewable here.

If there are any deficiencies in this pull request, please let me know so I could take additional action.

@kevmoo kevmoo requested a review from natebosch May 6, 2022 15:15
@kevmoo
Copy link
Member

kevmoo commented May 6, 2022

CC @brianquinlan

@kevmoo
Copy link
Member

kevmoo commented May 8, 2022

We have to be careful adding APIs here – they are breaking for common base classes.

@glassmonkey
Copy link
Author

I very much agree with that opinion.
I appreciate your thoughtful review.

As a matter of urgency, I'm going to use my own forked PATCH to get by, so no problem.

@natebosch
Copy link
Member

I think the main way this is breaking is for stubbed requests, I've found at least one case where redirects is throw UnimplementedError so this change causes existing tests to fail.

I think if we find it's only breaking for test code it may be OK to go forward with this change anyway, but we'll have to be careful during the rollout.

natebosch added a commit that referenced this pull request May 25, 2022
Closes #321, closes #623, closes #692
Fixes #293

Add a Uri field to BaseResponse with the final, potentially redirected,
url for the content.

The field is nullable for backwards compatibility - TODO: consider if
this should be non-nullable from the start, or if it can be published
nullable first and become non-nullable (and the constructor arg
required) in the next breaking release.

This may break tests which use bocks but do not mock the field used to
read the URL.
@natebosch natebosch mentioned this pull request May 25, 2022
@natebosch
Copy link
Member

If we are able to roll out this feature I think we'll use a Uri field instead of String. I'll pick this up in #699

@natebosch natebosch closed this May 25, 2022
@brianquinlan brianquinlan mentioned this pull request Jan 9, 2024
1 task
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.

3 participants