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

Record intermediate redirect locations #2093

Merged
merged 6 commits into from Sep 24, 2018

Conversation

Projects
None yet
3 participants
@marvelm
Contributor

marvelm commented Sep 16, 2018

Hi!

I've added tracking redirect locations in the response attributes. I've also pulled the status code handling logic into a separate function.

marvelm added some commits Sep 9, 2018

Add redirect URIs to response attributes
Add test to assert that intermediate URIs are returned

Run response transform after prepareLoop

Use the top-level test app

Move attribute key closer to accessor function

Fix test name

Add documentation

@marvelm marvelm force-pushed the marvelm:track-redirects branch from 7259e37 to 4fab0cd Sep 16, 2018

@rossabaker

This looks good to me, but I've been on a plane for five hours and I dont understand why this isn't building the list of redirects in reverse order. It prepends, but the test shows 1, 2, 3.

I will look again when I'm less tired.

@marvelm

This comment has been minimized.

Contributor

marvelm commented Sep 21, 2018

@rossabaker Nice catch! Prepending the list threw me for a spin too and I only wrote the feature a few days ago 🤦‍♂️ . I've added a comment to remind the reader that function is recursive.

@rossabaker

Oh, right. Which works out nicely for List. Neat!

This seems binary compatible to me. We can backport this if you'd like this in the 0.18 series.

@rossabaker

This comment has been minimized.

Member

rossabaker commented Sep 21, 2018

The build is failing because we need to run test:scalafmt.

@rossabaker rossabaker added this to the 0.19.0-M3 milestone Sep 23, 2018

@cquiroz

LGTM

@rossabaker rossabaker merged commit 2c4454e into http4s:master Sep 24, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment