Skip to content
This repository has been archived by the owner on Apr 8, 2020. It is now read-only.

Adding support for capturing the output of a node instance for custom… #184

Closed
wants to merge 1 commit into from
Closed

Adding support for capturing the output of a node instance for custom… #184

wants to merge 1 commit into from

Conversation

pauldotknopf
Copy link
Contributor

… logging implementations.

Not all implementations rely on stdout for logging and error messages.

@laskoviymishka
Copy link
Contributor

laskoviymishka commented Jul 15, 2016

Great point, best way to handle it IMHO to replace every Console.WriteLine to ILogger.Write, and inject logger from DI. Does it make sense for you?

@pauldotknopf
Copy link
Contributor Author

The INodeInstances are created via new (see here). No room for DI there.

@pauldotknopf
Copy link
Contributor Author

Are you referring to using the Microsoft libraries for logging?

@laskoviymishka
Copy link
Contributor

Yep, it is. And yep it need to be refactored :)

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Jul 18, 2016

Thanks @pauldotknopf! This is now merged (commit 27ffa7).

Instead of the custom INodeInstanceOutputLogger, as as per @laskoviymishka's comments, I've done a bit of further work to make it use the standard .NET ILogger interface, and to integrate it with DI.

The various INodeInstance subclasses now require you to pass an ILogger to them, and the DI-based instantiation flow uses the service provider to get an ILogger according to your config. If no logger is configured, it falls back on ConsoleLogger.

Hope this meets your needs, but please let me know if you have any concerns.

I haven't yet actually published updated NuGet packages containing this, as I'm waiting first to see if anyone thinks there's any problem with this.

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

Successfully merging this pull request may close these issues.

None yet

4 participants