Skip to content

Conversation

@chlowell
Copy link
Member

Today, Pipeline calls isinstance(policy, SansIOHTTPPolicy) to decide whether to wrap a policy with a runner defining send(). Explicit type checking is contrary to our guidelines' preference for structural subtyping and it may also be useful to allow a SansIOHTTPPolicy to define its own send(). So, this PR replaces the isinstance call with structural checks: if a policy implements send(), Pipeline calls that method; otherwise, if a policy implements SansIOHTTPPolicy's methods, Pipeline wraps that policy with a runner. If an alleged policy doesn't define any of these methods, e.g. it's None, Pipeline ignores it.

Copy link
Member

Choose a reason for hiding this comment

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

Guess this deserves a else now that raises that it won't work. Before, we were appended the policy and praying and it would have failed at the first "pipeline.run". Now, it silently drops the object passed as policy (silent drops are dangerous)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, that's reasonable. Breaks a few (hacky) tests for Key Vault (fixed by #16784), we'll see what else. Pipeline still needs to silently ignore None in policy lists because Configuration uses that as a default value for policies; I added test coverage of that.

Copy link
Member

Choose a reason for hiding this comment

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

Should we have this in policies/_base.py to make it live with SansIOHTTPPolicy definition?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, have done.

@chlowell
Copy link
Member Author

Closing this because it's no longer blocking anything.

@chlowell chlowell closed this Aug 17, 2021
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