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.

Sign up for GitHub

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

Jump to bottom

removed un-needed Ref from Client #6653

Closed
wants to merge 11 commits into from
Closed

removed un-needed Ref from Client #6653

wants to merge 11 commits into from

Conversation

geoffjohn11
Copy link

PR for #6326
I'm not entirely sure this is the right direction, but though I would take a try at it.

http4s-steward bot and others added 11 commits August 23, 2022 16:38
@http4s-steward
Update http4s-circe, http4s-ember-client to 0.23.15 in series/0.23
8df3b58
@armanbilge
Merge pull request http4s#6635 from http4s/update/series/0.23/http4s-…
edaf32c 
…circe-0.23.15

Update http4s-circe, http4s-ember-client to 0.23.15 in series/0.23
@http4s-steward
Update netty-buffer, netty-codec-http to 4.1.80.Final in series/0.23
14f961c
@armanbilge
Merge pull request http4s#6640 from http4s/update/series/0.23/netty-b…
0bcece7 
…uffer-4.1.80.Final

Update netty-buffer, netty-codec-http to 4.1.80.Final in series/0.23
@http4s-steward
Update case-insensitive, ... to 1.3.0 in series/0.23
d6b998e
@armanbilge
Merge pull request http4s#6642 from http4s/update/series/0.23/case-in…
264f2f0 
…sensitive-1.3.0

Update case-insensitive, ... to 1.3.0 in series/0.23
@armanbilge
Pin slf4j to 1.x and cleanup [ci skip]
9d86185
@danicheg
Merge pull request http4s#6644 from http4s/steward-pin-sl4fj
f49e068 
Pin slf4j to 1.x and cleanup
@danicheg
Pin logback-classic to the 1.2.x series
6d2c502
@danicheg
Merge pull request http4s#6647 from danicheg/pin-logback-classic
57cedc0 
Pin `logback-classic` to the `1.2.x` series
@geoffjohn11
removed un-needed Ref from Client
e04c013
@mergify mergify bot added series/0.23 PRs targeting 0.23.x module:client labels Sep 6, 2022
@armanbilge
Copy link
Member

Thanks for the PR! I think the original proposal was in #6325 (review).

I don't think this needs the Ref unless the body is a Stream, but this is progress.

In series/0.23 the body is always a Stream so I am not sure if we can make this change here. However in main we have redefined the Entity model so that the body is not always a Stream. In those cases we do not need the Ref.

@geoffjohn11
Copy link
Author

Thanks for the PR! I think the original proposal was in #6325 (review).

I don't think this needs the Ref unless the body is a Stream, but this is progress.

In series/0.23 the body is always a Stream so I am not sure if we can make this change here. However in main we have redefined the Entity model so that the body is not always a Stream. In those cases we do not need the Ref.

Thanks! I'll take a look at the main branch and see if I can come up with the right idea.

@geoffjohn11 geoffjohn11 changed the base branch from series/0.23 to main September 6, 2022 22:47
@geoffjohn11 geoffjohn11 closed this Sep 6, 2022
@geoffjohn11
Copy link
Author

Committed to wrong branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
module:client series/0.23 PRs targeting 0.23.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@geoffjohn11 @armanbilge @danicheg