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

Generic message type <T> #18

Open
alexis- opened this issue Apr 21, 2022 · 4 comments
Open

Generic message type <T> #18

alexis- opened this issue Apr 21, 2022 · 4 comments

Comments

@alexis-
Copy link

alexis- commented Apr 21, 2022

I have noticed that all the read/write APIs can only read and write the generic type defined by the user when instantiating the client and server, eg. PipeServer<MyMessage>, PipeClient<MyMessage>.

Would you agree to a PR with the following changes:

  • PipeClient, PipeServer, SingleConnectionPipeServer, SingleConnectionPipeClient would have a generic and non-generic variant
  • The non-generic variant would be the base class for the generic variant to avoid code duplication
  • The non-generic variant would expose the following methods for writing:
    • void WriteAsync<T>(T value, [...])
    • void WriteAsync(byte[] value, [...])
  • The non-generic variant would expose the following event for receiving data event EventHandler<ConnectionMessageEventArgs<byte[]>>? MessageReceived;
  • The generic variant would be the same as the current PipeXXX
  • One of the following:
    • SingleConnectionPipeXXX and PipeXXX appear to share a lot of code. They could inherit an abstract class for all the code they have in common
    • Merge SingleConnectionPipeXXX and PipeXXX by adding a bool isSingleConnection argument to the constructor
  • Optionally make the classes non-sealed (more on that below)

In essence we would have something like that:

Avoiding code duplication between SingleConnectionPipeXXX and PipeXXX

Option 1: Base class

public abstract BasePipeServer : IPipeServer
{
  // Put all code shared by PipeServer and SingleConnectionPipeServer here
}
public abstract BasePipeClient : IPipeClient
{
  // Put all code shared by PipeClient and SingleConnectionPipeClient here
}

Option 2: Merge them

We delete the SingleConnectionPipeXXX classes and add an argument in the constructor:

public PipeServer(string pipeName, bool isSingleConnection = false, IFormatter? formatter = default)
public PipeClient(string pipeName, string serverName = ".", bool isSingleConnection = false, TimeSpan? reconnectionInterval = default, IFormatter? formatter = default)

Non-generic PipeXXX classes

public class PipeServer: BasePipeServer
{
  public event EventHandler<ConnectionMessageEventArgs<byte[]>>? MessageReceived;

  public async Task WriteAsync<T>(T value, CancellationToken cancellationToken = default)
  { /* Code here */ }

  public async Task WriteAsync(byte[] value, CancellationToken cancellationToken = default)
  { /* Code here */ }
}
public class PipeClient: BasePipeClient
{
  public event EventHandler<ConnectionMessageEventArgs<byte[]>>? MessageReceived;

  public async Task WriteAsync<T>(T value, CancellationToken cancellationToken = default)
  { /* Code here */ }

  public async Task WriteAsync(byte[] value, CancellationToken cancellationToken = default)
  { /* Code here */ }
}

Repeat for SingleConnectionPipeXXX.

Generic PipeXXX classes

public class PipeServer<T>: PipeServer, IPipeServer<T>
{
  public new event EventHandler<ConnectionMessageEventArgs<T?>>? MessageReceived;

  public new async Task WriteAsync(T value, CancellationToken cancellationToken = default)
  { /* Code here */ }
}
public class PipeClient<T>: PipeClient, IPipeClient<T>
{
  public new event EventHandler<ConnectionMessageEventArgs<T?>>? MessageReceived;

  public new async Task WriteAsync(T value, CancellationToken cancellationToken = default)
  { /* Code here */ }
}

Repeat for SingleConnectionPipeXXX.

Optional: Sealed classes

Optionally, it might be nice to make the PipeServer<T> and PipeClient<T> non-sealed for people who want to extend their functionalities without requiring a fork or a PR.

Optional: Sharing code between PipeServer and PipeClient

PipeServer and PipeClient also appear to share a lot of code. I could make a base class for them to avoid code duplication.

Final words

I'll be writing this for my own needs, but I would enjoy contributing to the project if possible with a PR. Let me know what you think! :)

@HavenDV
Copy link
Owner

HavenDV commented Apr 22, 2022

Hello. Thanks for the offer, I'm not against it, it should have been done a long time ago.
You will also need to make sure that tests with the current behavior of the Generic classes pass correctly and the behavior does not change.
You will also need to carefully approach the issue of combining SingleConnectionPipeXXX and PipeXXX, because I do not remember the reasons for my decision to split instead of adding a parameter (but, there is a chance that they were not there and it was corny laziness)
I also don't mind all Optional changes.

The only thing I'll forgive - divide the implementation into separate PRs as much as possible to simplify verification. So that there is no situation where there is no code comparison, but there are simply deleted and added files.

@alexis-
Copy link
Author

alexis- commented Apr 22, 2022

Sounds good! Glad you're onboard. :)

I'll start with the generic/non-generic PipeXXX modification + tests and send a PR when it's finished.

@fectus
Copy link

fectus commented May 17, 2023

This feature looks cool and would help me a lot to achieve polymorphic pipe messages. Apparently, the PR did not make it through though. And since it has been more than a year now the work/review has been put on hold probably.

For now, I will have to stick to sending content serialized as JSON for example and handle the JSON deserialization at the receiving side.
Anyways, great and very helpful library and a very nice addition to the library API if the PR was completed.

@HavenDV
Copy link
Owner

HavenDV commented May 17, 2023

I still love this suggestion. Unfortunately, the PR was quite voluminous, I was not able to quickly understand it and therefore it is still not accepted

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

No branches or pull requests

3 participants