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

[Feature request]: StrategyHelper, DisposeHelper, TimeProviderExtensions, TaskHelper should be public not internal #2039

Closed
mhsimkin opened this issue Apr 1, 2024 · 6 comments

Comments

@mhsimkin
Copy link

mhsimkin commented Apr 1, 2024

Is your feature request related to a specific problem? Or an existing feature?

If you need to add a new strategy and want to use StrategyHelper.ExecuteCallbackSafeAsync, DisposeHelper.TryDisposeSafeAsync, TimeProviderExtensions::DelayAsync, TaskHelper::GetResult you can't as they are marked internal.

Describe the solution you'd like

StrategyHelper, DisposeHelper, TimeProviderExtensions should be made public.

Additional context

Referencing this discussion with @peter-csala. After taking the provided code and building a simple test application I determined that the functionality provided by chaining strategies didn't work the way I needed.

I decided to build my own strategy, using the RetryResilienceStrategy as a starting point. Unfortunately, the three (3) classes indicated above are marked as internal, therefore I had to create my own implementation.

I should be able to use these classes in my strategy to minimize my introduction in bugs and to allow the Polly team to resolve any issues that might requite these methods in the indicated classes to be updated.

@mhsimkin mhsimkin changed the title [Feature request]: StrategyHelper, DisposeHelper, TimeProviderExtensions should be public not internal [Feature request]: StrategyHelper, DisposeHelper, TimeProviderExtensions, TaskHelper should be public not internal Apr 1, 2024
@martincostello
Copy link
Member

We can consider making it easier for people to extend things, but it's unlikely we'd expose these types as-is.

@peter-csala
Copy link
Contributor

I determined that the functionality provided by chaining strategies didn't work the way I needed.

Could you please elaborate what is missing? How do you want to implement your own custom retry strategy?

@mhsimkin
Copy link
Author

mhsimkin commented Apr 2, 2024

I will add a reply to the discuss of what the issues were.

@martintmk
Copy link
Contributor

I agree with @martincostello , it's unlikely that we will expose these primitives as we want to keep our API surface as thin as possible. Most of these are quite simple, so copy & paste is your friend.

@peter-csala
Copy link
Contributor

peter-csala commented Apr 16, 2024

@mhsimkin Hi Marc, as you have indicated in the discussion the proposed solution worked for you. Can you please close this issue because as far as I can tell your feature request is not necessary anymore.

I'm planning to add the code to the samples and document it under the extensibility section next week.

@mhsimkin
Copy link
Author

This is no longer needed as @peter-csala and I have come up with a working solution. Please see this discussion for the details.

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

No branches or pull requests

4 participants