Refine PolicyWrap configuration syntax to take parameters as interfac… #302

rjongeneelen · 2017-08-25T19:58:17Z

…es #297

dnfclas · 2017-08-25T19:58:21Z

@ExtRemo75,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

rjongeneelen · 2017-08-25T20:07:23Z

Used interfaces throughout PolicyWrap logic, this required splitting some methods as well whenusing IsPolicy
Extended ISync/IAsync interfaces with access to Predicates + made those delegates public
Rewrote PolicyWrap static Wrap/WrapAsync methods to stop making use of Policy.Wrap() methods, since this would require making PolicyWrap public as well, which seems unnecessary and unwanted. It also removes PolicyWrap-specific logic from the Policy class
Changed static Success/Failure methods of PolicyResult to public, to enable instantiation from a derived policy

udlose · 2017-09-14T03:53:52Z

src/Polly.Shared/Wrap/PolicyWrap.cs

@@ -21,12 +21,12 @@ internal PolicyWrap(Action<Action<Context, CancellationToken>, Context, Cancella
    /// <typeparam name="TResult">The return type of delegates which may be executed through the policy.</typeparam>
    public partial class PolicyWrap<TResult> : Policy<TResult>, IPolicyWrap<TResult>
    {
-        internal PolicyWrap(Func<Func<Context, CancellationToken, TResult>, Context, CancellationToken, TResult> policyAction, Policy outer, IsPolicy inner)
+        internal PolicyWrap(Func<Func<Context, CancellationToken, TResult>, Context, CancellationToken, TResult> policyAction, ISyncPolicy outer, ISyncPolicy<TResult> inner)
            : base(policyAction, outer.ExceptionPredicates,  PredicateHelper<TResult>.EmptyResultPredicates)


Shouldn't both of these be ISyncPolicy?

Just like in PolicyWrapAsync.cs, there should be 3 variants:
1: ..., ISyncPolicy outer, ISyncPolicy<TResult> inner)
2: ..., ISyncPolicy<TResult> outer, ISyncPolicy inner)
3: ..., ISyncPolicy<TResult> outer, ISyncPolicy<TResult> inner)
As you mentioned in your next review-comment below, I accidentally forgot to split up 2 and 3, I have pushed the changes to commit b3cc1a7.

udlose · 2017-09-14T03:57:52Z

src/Polly.Shared/Wrap/PolicyWrap.cs

            : base(policyAction, outer.ExceptionPredicates,  PredicateHelper<TResult>.EmptyResultPredicates)
        {
        }

-        internal PolicyWrap(Func<Func<Context, CancellationToken, TResult>, Context, CancellationToken, TResult> policyAction, Policy<TResult> outer, IsPolicy inner)
+        internal PolicyWrap(Func<Func<Context, CancellationToken, TResult>, Context, CancellationToken, TResult> policyAction, ISyncPolicy<TResult> outer, IsPolicy inner)


Why was the 3rd param left as type IsPolicy? Not consistent with your other changes

Good observation, thanks!

Just like in PolicyWrapAsync.cs, there should be 3 variants:
1: ..., ISyncPolicy outer, ISyncPolicy<TResult> inner)
2: ..., ISyncPolicy<TResult> outer, ISyncPolicy inner)
3: ..., ISyncPolicy<TResult> outer, ISyncPolicy<TResult> inner)

I will split up the IsPolicy into 2 and 3, which also aligns it with the changes made to PolicyWrapAsync.cs.

I have pushed the changes to commit b3cc1a7.

udlose · 2017-09-14T04:05:26Z

src/Polly.Shared/Wrap/PolicyWrapSyntax.cs

        {
            switch (policies.Count())
            {
                case 0:
                case 1:
                    throw new ArgumentException("The enumerable of policies to form the wrap must contain at least two policies.", nameof(policies));
                default:
-                    IEnumerable<Policy> remainder = policies.Skip(1);
-                    return policies.First().Wrap(remainder.Count() == 1 ? remainder.Single() : Wrap(remainder.ToArray()));
+                    ISyncPolicy[] remainder = policies.Skip(1).ToArray();


Why add the overhead of creating an array using ToArray()? Use IEnumerable<ISyncPolicy>

In the old code, the enumerable already was converted into an array when passing it as argument to the policies.First().Wrap(...) method. All I did was move the ToArray to an earlier moment, to prevent both the Count() and the ToArray() to navigate through the enumerable. It should result to less overhead rather than more.

If we want to remove the ToArray, I reckon these type of performance improvements should be part of a separate pull request?

udlose · 2017-09-14T04:08:00Z

src/Polly.Shared/Wrap/PolicyWrapSyntaxAsync.cs

        {
            switch (policies.Count())
            {
                case 0:
                case 1:
                    throw new ArgumentException("The enumerable of policies to form the wrap must contain at least two policies.", nameof(policies));
                default:
-                    IEnumerable<Policy> remainder = policies.Skip(1);
-                    return policies.First().WrapAsync(remainder.Count() == 1 ? remainder.Single() : WrapAsync(remainder.ToArray()));
+                    IAsyncPolicy[] remainder = policies.Skip(1).ToArray();


Same comment as above about ToArray()

udlose · 2017-09-14T04:08:57Z

src/Polly.Shared/Wrap/PolicyWrapSyntaxAsync.cs

        {
            switch (policies.Count())
            {
                case 0:
                case 1:
                    throw new ArgumentException("The enumerable of policies to form the wrap must contain at least two policies.", nameof(policies));
                default:
-                    IEnumerable<Policy<TResult>> remainder = policies.Skip(1);
-                    return policies.First().WrapAsync(remainder.Count() == 1 ? remainder.Single() : WrapAsync(remainder.ToArray()));
+                    IAsyncPolicy<TResult>[] remainder = policies.Skip(1).ToArray();


Same comment as above about ToArray()

rjongeneelen · 2017-09-14T15:53:01Z

@reisenberger Not sure why the build fails, looks like a generic build error, rather than related to my changes?

reisenberger · 2017-09-15T06:00:24Z

Huge thank you, @ExtRemo75 , for your contribution, and thank you @udlose, @ExtRemo75 , both, for your close attention to this code! Yes, that corner of the PolicyWrap config code was on my list to tidy: please feel free!

(Apologies that vacation followed by tight commitments have kept me from these PRs just lately ...)

reisenberger · 2017-09-19T11:29:50Z

Cross-ref build issue

reisenberger · 2017-10-21T14:42:36Z

@ExtRemo75 Huge thanks for this contribution (just reviewed again)

I am going to pend this PR for the moment, as I am also exploring #281 as part of v6, which may make the sync/async split redundant.

(Thanks also for the attention to the PolicyWrap config code!)

reisenberger · 2017-11-24T01:25:16Z

Closing in favour of #370 , which already merged to the v560dev branch, because this (#302) also included parts of #299 (which not ready to merge/still under discussion).

Credit added to @rjongeneelen in the readme, for doing much of the refinement on PolicyWrap config syntax, which also found its way into #370.

#370 also addressed some of the useful comments on further cleanups for PolicyWrap syntax arising in discussion from @udlose and @rjongeneelen , thanks y'all!

Refine PolicyWrap configuration syntax to take parameters as interfaces

27e60b7

App-vNext#297

dnfclas added the cla-already-signed label Aug 25, 2017

udlose reviewed Sep 14, 2017

View reviewed changes

Fix inconsistent method signatures

b3cc1a7

Merge branch 'master' into Interfaces

ce87e10

reisenberger added the on hold label Oct 25, 2017

reisenberger mentioned this pull request Nov 24, 2017

Make PolicyWrap configuration overloads take parameters as interfaces #370

Merged

reisenberger closed this Nov 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refine PolicyWrap configuration syntax to take parameters as interfac… #302

Refine PolicyWrap configuration syntax to take parameters as interfac… #302

rjongeneelen commented Aug 25, 2017

dnfclas commented Aug 25, 2017

rjongeneelen commented Aug 25, 2017 •

edited

Loading

udlose Sep 14, 2017

rjongeneelen Sep 14, 2017 •

edited

Loading

udlose Sep 14, 2017

rjongeneelen Sep 14, 2017 •

edited

Loading

udlose Sep 14, 2017

rjongeneelen Sep 14, 2017

udlose Sep 14, 2017

rjongeneelen Sep 14, 2017

udlose Sep 14, 2017

rjongeneelen Sep 14, 2017

rjongeneelen commented Sep 14, 2017 •

edited

Loading

reisenberger commented Sep 15, 2017

reisenberger commented Sep 19, 2017

reisenberger commented Oct 21, 2017

reisenberger commented Nov 24, 2017

Refine PolicyWrap configuration syntax to take parameters as interfac… #302

Refine PolicyWrap configuration syntax to take parameters as interfac… #302

Conversation

rjongeneelen commented Aug 25, 2017

dnfclas commented Aug 25, 2017

rjongeneelen commented Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

rjongeneelen Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjongeneelen Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjongeneelen commented Sep 14, 2017 • edited Loading

reisenberger commented Sep 15, 2017

reisenberger commented Sep 19, 2017

reisenberger commented Oct 21, 2017

reisenberger commented Nov 24, 2017

rjongeneelen commented Aug 25, 2017 •

edited

Loading

rjongeneelen Sep 14, 2017 •

edited

Loading

rjongeneelen Sep 14, 2017 •

edited

Loading

rjongeneelen commented Sep 14, 2017 •

edited

Loading