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 copy constructor to RequestConfiguration{Descriptor} #133

Merged
merged 2 commits into from
Nov 13, 2024

Conversation

flobernd
Copy link
Member

@flobernd flobernd commented Nov 8, 2024

As titled.

What is the deal with DisablePing, EnableHttpPipelining in RequestConfiguration (both properties are not part of the interface) and _enableHttpCompression and _maxRetryTimeout in RequestConfigurationDescriptor (both fields don't have a corresponding setter-method)?

Is that by design or a leftover of the refactoring?

@stevejgordon
Copy link
Collaborator

stevejgordon commented Nov 8, 2024

As titled.

What is the deal with DisablePing, EnableHttpPipelining in RequestConfiguration (both properties are not part of the interface) and _enableHttpCompression and _maxRetryTimeout in RequestConfigurationDescriptor (both fields don't have a corresponding setter-method)?

Is that by design or a leftover of the refactoring?

I think some of those need cleaning/fixing up. DisablePing, I think, was renamed to DisablePings and EnabledHttpPipelining to HttpPipeliningEnabled. My guess is _enableHttpCompression and _maxRetryTimeout should have setter methods.

Copy link
Member

@Mpdreamz Mpdreamz left a comment

Choose a reason for hiding this comment

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

Echo'ing what Steve said, the RequestCongfiguration and RequestData need another pass to ensure conformity.

Happy to take that on in another PR :)

For this PR can we add another test to ensure all properties get passed through similar to ITransportConfiguration ?

@@ -58,7 +58,7 @@ private RequestConfiguration PingAndSniffRequestConfiguration
PingTimeout = PingTimeout,
RequestTimeout = PingTimeout,
Authentication = _requestData.AuthenticationHeader,
EnableHttpPipelining = _requestData.HttpPipeliningEnabled,
Copy link
Member Author

Choose a reason for hiding this comment

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

That property was not wired at all 😅

@flobernd
Copy link
Member Author

I removed the properties which are not present in the interface, added methods for the ones that ARE present in the interface but were not settable in the descriptor, and added some tests for the RequestConfiguration{Descriptor}.

@flobernd flobernd requested a review from Mpdreamz November 13, 2024 12:17
Copy link
Collaborator

@stevejgordon stevejgordon left a comment

Choose a reason for hiding this comment

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

LGTM - thanks

@flobernd flobernd merged commit 1fbc364 into main Nov 13, 2024
5 checks passed
@flobernd flobernd deleted the req-config-desc-copy-ctor branch November 13, 2024 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants