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

Added Option to add Interceptors on Client Level #2052

Closed
wants to merge 20 commits into from

Conversation

fseidl-bauradar
Copy link
Contributor

Description

Added Possibility to add Client wide Interceptors, to check Request and Responses on multiple positions

Purpose

This pull request is a:

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

using System.Text;

namespace RestSharp.Interceptors {
public interface IBeforeSerializationInterceptor {
Copy link
Member

Choose a reason for hiding this comment

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

I still don't understand why these interfaces are needed as they can't be used. I am also not sure what's the purpose of the IInterceptor interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original purpose of this Interfaces was to make the Interceptor compatible with your delegates, so the three Interceptors can be assigned to the Request to.

In the first step, I thought about combining the handling of the Request Delegates with the handling of the Interceptors.

var List beforeRequestInterceptors = new();
beforeRequestInterceptors.add(request.OnBeforeRequest);
beforeRequestInterceptors.add(Options.Interceptors);
foreach(interceptor in beforeInterceptors){
await interceptor.InterceptBeforeRequest(request);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

class BeforeRequestInterceptor: IBeforeRequestInterceptor {
...
}
var beforeRequestInterceptor = new BeforeRequestInterceptor();
request.OnBeforeRequestInterceptor += beforeRequestInterceptor;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another reason why I added the IInterceptor Interface, was that I don't like adding Classes as Parameters, I think it's always better to use Interfaces instead.

Copy link
Member

Choose a reason for hiding this comment

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

Another reason why I added the IInterceptor Interface, was that I don't like adding Classes as Parameters, I think it's always better to use Interfaces instead.

Why? I don't think code is about what we like or don't. Each line of code potentially adds complexity and certainly adds maintenance. Using interfaces is one of the ways in .NET for creating abstractions, and if you don't plan for the abstraction to be used, there's no need for it.

Regarding separate collections for different interjectors, I'd prefer making the existing ones obsolete and just remove them later. I don't want to keep redundant functionality, and interceptors are indeed much cleaner and more idiomatic way to do the same thing as those delegates are doing now.

Copy link
Contributor Author

@fseidl-bauradar fseidl-bauradar Apr 5, 2023

Choose a reason for hiding this comment

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

For sure, each Line of Code adds Complexity, but interfaces are just a promise what a implementing class can do,
There is no working Code inside them, so they don't increase the complexity.

Interfaces provide the Option to create a complete own implementation

If you create a Interface you know that there is nothing done in the implementation, but if you derive from a super class, there can be Implementations that disturb your way of working with them.

Add multiple Interfaces to a class but only one super class.

Copy link
Contributor Author

@fseidl-bauradar fseidl-bauradar Apr 19, 2023

Choose a reason for hiding this comment

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

Is there anyway that we can get this Changes in the offizial Repository?
If you want I can remove the interfaces, even though I don't think interfaces are much more affort that without them.
It's complicate to keep the fork synchronized, creating a pull request every time when synchronizing to the original.

@fseidl-bauradar
Copy link
Contributor Author

Because there have been commits in the Branch and I need to synchronize

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

Successfully merging this pull request may close these issues.

2 participants