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

Simplify code in Follow Redirect: No Options #2574

Merged
merged 1 commit into from Jun 10, 2019

Conversation

@diesalbla
Copy link
Contributor

commented May 16, 2019

Some changes to simplify the code in FollowRedirect by removing false options.

  • The variable pureBody was defined as a Some, so there was no need for the Option in its type.
  • The Method nextRequest was identical in both branches except for the last instruction, about the body, which depends on whether bodyOpt is a Some or a None
  • The bodyOpt parameter was fixed in each call to either a Some or a None, which means we can fold last part of the def nextRequest method after the call.
  • The nextRequest was an Option only because the pureBody was. Since pureBody is always Some, we can remove branching there.
  • And thus remove the nextReq.fold(dontRedirect) at the end.
@rossabaker
Copy link
Member

left a comment

Looks good to me, but I think one of those comments is outdated as well.

@diesalbla

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

@rossabaker After digging a bit deeper, I ended up with a change a bit larger that what I started with at first. I have extracted a pure, non-effectful, non-optional function nextRequest, which carries the whole process of building the redirection-request from the previous request and response.

@diesalbla diesalbla force-pushed the diesalbla:FollowRedirect_simplify branch 2 times, most recently from dca16ea to 927415f Jun 6, 2019

Nope, the tests are failing.

@rossabaker

This comment has been minimized.

Copy link
Member

commented Jun 9, 2019

The test failures look related. I think something broke in the refactoring.

@diesalbla diesalbla force-pushed the diesalbla:FollowRedirect_simplify branch from 927415f to 8c991b2 Jun 9, 2019

Simplify code in Follow Redirect: No Options
- The variable `pureBody` was defined as a `Some`, so there was no
  need for the `Option` in its type.
- The Method `nextRequest` was identical in both branches except for
  the last instruction, about the body, which depends on whether
  `bodyOpt` is a `Some` or a `None`
- The `bodyOpt` parameter was fixed in each call to either a `Some` or
  a `None`, which means we can fold last branch into it.
- The `nextRequest` was an Option only because the `pureBody` was.
  Since `pureBody` is always `Some`, we can remove branching there.
- And thus remove the `nextReq.fold(dontRedirect)` at the end.

Then we Restructure the code of Follow Redirect.

- We extract a pure function `nextRequest`, which keeps the whole
  process of building the request for each next step as a pure function.

- We reduce all the conditions on whether to keep redirecting or not
  into a single pattern-match with condition and default.
  This allows us to inline the base case (do not redirect).

@diesalbla diesalbla force-pushed the diesalbla:FollowRedirect_simplify branch from 8c991b2 to fc759bb Jun 9, 2019

@diesalbla

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2019

There was a mistake in composing the operations of nextRequest, which I have fixed.

@rossabaker
Copy link
Member

left a comment

Thanks!

In the future, it's a little easier to just push new commits instead of force pushing the whole PR, so we don't have to reexamine the whole diff.

@aeons aeons merged commit 579b980 into http4s:master Jun 10, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@diesalbla diesalbla deleted the diesalbla:FollowRedirect_simplify branch Jun 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.