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

Change the access modifiers of Policy constructors, methods and relat… #299

Closed

Conversation

rjongeneelen
Copy link
Contributor

…ed properties. Make Policy properties/methods virtual so they can be overridden.

See issue #298

…ed properties. Make Policy properties/methods virtual so they can be overridden.
@reisenberger
Copy link
Member

reisenberger commented Oct 21, 2017

@extremo75 , Apologies for the delay responding. Huge thank-you for your work on this PR towards allowing custom policies!

The PR goes further than the original proposal (thanks for your note to this effect!). My slight concern is it (at mo) creates a very broad extension surface to Polly, which could cause problems down the line for both Polly and users extending. It may be better for both if we can achieve the same extensibility but keep extension points more tightly focused?

The concern over a broad extension surface is the large number of potential points of contact between Polly and custom-policies which it creates - a join with a large number of connection points is potentially more rigid/ inflexible/ brittle, as Polly evolves, than if we can create one with only a small number. With many extension points, if we change only a single one, it risks a breakage for users with custom policies. Equally, if we set a precedent that almost every method on Policy is public/protected virtual so that people could override it, users who've created custom policies this way risk having frequently to 'play catch up' (override more methods), as we evolve and extend. (Items like ((EDIT)) 281 will cause significant changes.)

A single extension point (or tightly focused group) - like just making the private base class constructors protected, and making public anything necesssary to return results - should otoh flex more easily with Polly as it grows?

I hope this makes sense. What do you think?


So, practically, the main concern was making the .Execute(...) overloads override-able. To make sure I've understood what this would help users achieve: what were the particular use cases you envisaged for overriding .Execute(...) and related overloads? To permit custom policies, I had envisaged it might be enough just to allow a custom implementation (passed into the base Policy constructor at the moment), and let the .Execute(...) overloads do their normal job.

Thanks again for all your contribution to this!

@reisenberger
Copy link
Member

Closed in favour of the simpler equivalent, #391

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.

4 participants