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

Suggestion for a less complicated API. #962

Closed

Conversation

sxleixer
Copy link

@sxleixer sxleixer commented Oct 2, 2022

This should not be integrated into the master, you should just be able to see the deltas I've done in my branch. This PR is related to #961 demonstrating how it would feel to have interfaces instead of delegates to work with.

I think the impact on the perfomance should be miniscule if not null.

This approach should also be expanded to the TimeProvider and other functions. Please feel free to reach out to me.

@dnfadmin
Copy link

dnfadmin commented Oct 2, 2022

CLA assistant check
All CLA requirements met.

Copy link
Member

@martincostello martincostello left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request.

The "explosion" of parameters in the way to configure the policies has come up several times over the last year or so in issues, and it's something that's being considered for change in v8 (whenever that is).

Thoughts being thrown around have included removing all the different overloads/combinations, and replacing them with a single "options"/"settings" value that all the different possible configurations can be specified in a single place.

I don't think we'd want to add something like this in a v7.x as it just adds even more code and complexity to what we already have.

src/Polly/Bulkhead/BulkheadPolicy.cs Outdated Show resolved Hide resolved
src/Polly/Bulkhead/BulkheadPolicy.cs Outdated Show resolved Hide resolved
src/Polly/Bulkhead/BulkheadSyntax.cs Outdated Show resolved Hide resolved
src/Polly/Bulkhead/BulkheadTResultSyntax.cs Outdated Show resolved Hide resolved
src/Polly/Bulkhead/Settings/BulkheadPolicySettings.cs Outdated Show resolved Hide resolved
/// The callback object which handles the callback.
/// </summary>
/// <remarks>null means default</remarks>
public IBulkheadRejectedCallback OnBulkheadRejectedCallback { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Personally I think it's weird to have an interface be named callback, I would expect a delegate for such a name.

src/Polly/Retry/Settings/DelegateOnRetryCallback.cs Outdated Show resolved Hide resolved
@martincostello martincostello marked this pull request as draft October 3, 2022 07:49
@sxleixer
Copy link
Author

sxleixer commented Oct 3, 2022

I made some updates upon your comments. Renaming some of the files. Added a proposal for type-based range checking (see e.g. RetryCountValue). Expanded the example for the non-async RetryPolicy.

@martincostello
Copy link
Member

Development is now underway for Polly v8, where we hope the new API addresses various concerns about an over-complicated API surface - see #1048 for more information.

I'm going to close this issue due to this work and a lack of activity. Feel free to raise suggestions related to the new design if you have any suggestions that aren't covered by the new API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants