grpc-dotnet-namedpipes
grpc-dotnet-namedpipes copied to clipboard
cyanfish
Handling of open logging todos
There are a couple of occurences in the code which have todo comments regarding logging handling.
Perhaps it would be best if the structure is changed a bit so that there is a client/server settings class which can pass on properties like bool ThrowsException and Action<Exception> Log
However upon initial investigation i noticed that there are some classes in the Internal folder which are not actually internal. Perhaps it would be best to have a proper architectural cleanup and fix issues like this together with it?
I think those classes can just be made internal with no issue.
As far as logging, my idea was to have an ILogger property on the options classes.
@cyanfish While i like that logging interface it would add a dependency on this library which isn't actually needed for it to run. Hence the Action<Exception>. Many other libraries take similar approaches to reduce dependencies
I'd prefer to have easier compatibility. Microsoft.Extensions.Logging is commonly used (e.g. in grpc-dotnet) and has adapters for pretty much every logging framework. Plus it's easy to use extra functionality (e.g. different log levels) if that's ever needed.
Yes, i understand what you are trying to do. But a library for communication forcing a logging framework down your throat is a big no no in library development. Check nuget for big libraries - you're not going to see any of them adding a dependency for logging.
Well, as far as big libraries, there's Grpc.Net.Client :). Specifically the package isn't a logging framework but an abstraction that can be used with any framework.
It's certainly a fair position to not want to depend on it; but for this project, if logging is added, I would like to stay in line with existing gRPC projects.
It's certainly a fair position to not want to depend on it; but for this project, if logging is added, I would like to stay in line with existing gRPC projects.
That makes perfect sense - one way or another better than the current state would already be throwing the exception and adding documentation tags to indicate that a class/method might throw an exception.
Yeah, actually some kind of Error event on the server/channel objects would probably do the trick.