Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

#42 added color option to ConsoleOutLoggerFactoryAdapter #43

Merged
merged 2 commits into from Jun 1, 2014

Conversation

Projects
None yet
3 participants
Contributor

adamralph commented Mar 3, 2014

fixes #42 using the 'new constructor for ConsoleOutLoggerFactoryAdapter' solution

Contributor

adamralph commented Mar 3, 2014

Hold off merging for now. I've just realised this implementation is flawed. I'll update the PR later.

Contributor

adamralph commented Mar 3, 2014

OK, I've replaced the commit. I think it's all good to go now, although the Sliverlight #if conditionals are quite cumbersome.

An alternative approach would be to introduce a ColoredConsoleOutLoggerFactoryAdapter and a ColoredConsoleOutLogger and add them only to the non-SL assemblies. If you'd rather go down that route then let me know and I can redo the feature in that manner.

@blairconrad blairconrad and 1 other commented on an outdated diff Mar 4, 2014

src/Common.Logging/Logging/Simple/ConsoleOutLogger.cs
@@ -61,6 +96,25 @@ protected override void WriteInternal(LogLevel level, object message, Exception
FormatOutput(sb, level, message, e);
// Print to the appropriate destination
+#if !SILVERLIGHT
+ if (this.useColor)
+ {
+ var originalColor = Console.ForegroundColor;
+ ConsoleColor color = originalColor;
+ colors.TryGetValue(level, out color);
@blairconrad

blairconrad Mar 4, 2014

If the level isn't found, color will become default(ConsoleColor), which is black. Based on the values in colors, I think we're assuming a dark (black?) background, so that's perhaps not ideal. Maybe better to detect the missing value and fall back to originalColor?

While I'm griping, no love for people with light-coloured backgrounds? 😉

@adamralph

adamralph Mar 4, 2014

Contributor

Does TryGetValue() specifically assign default(T) if the key is not found? I guess it might at least. I'll change it to be immune to that.

Light backgrounds - it crossed my mind too but for now I just went with replicating the default behaviour of the NLog ColoredConsoleTarget with regard to the chosen foreground colors as well as not altering the background color. I guess something clever to detect the lightness of the background and switch to a dark foreground scheme could be added later.

Contributor

adamralph commented Mar 4, 2014

image
image

Does TryGetValue() specifically assign default(T) if the key is not found?

That's how I read the docs.

And, yup. Look:

C:\PortableApps\Documents\Source>scriptcs
scriptcs (ctrl-c to exit)

> var dict = new Dictionary<string, System.ConsoleColor>();
> var originalColor = System.ConsoleColor.White;
> dict.TryGetValue("ERROR", out originalColor);
false
> originalColor
0
> (int)System.ConsoleColor.Black
0
> (int)System.ConsoleColor.White
15

I guess something clever to detect the lightness of the background and switch to a dark foreground scheme could be added later.

Yeah. I was just having a little fun with you on that one. Sorry.

Contributor

adamralph commented Mar 19, 2014

Has anyone had a chance to look at this PR? I'm happy to re-visit if required.

Owner

sbohlen commented Mar 31, 2014

@adamralph I'll take a look at it this week and let you know -- thanks for the contribution and sorry for the delayed response!

Contributor

adamralph commented May 2, 2014

😢

Owner

sbohlen commented May 2, 2014

Hang tight -- this weekend is "common.logging weekend" for me so I'll get this in shortly (promise). I've been traveling ridiculously-much for work the past ~30 days.

Contributor

adamralph commented May 2, 2014

😃

Sure, no probs. I know how it is...

Owner

sbohlen commented Jun 1, 2014

Per your comment ...

An alternative approach would be to introduce a ColoredConsoleOutLoggerFactoryAdapter and a ColoredConsoleOutLogger and add them only to the non-SL assemblies.

...could you re-jigger your PR to use this approach? Now that I've restructured the relationships betw the PCL assemblies and the non-PCL assemblies (so that there's no longer any dependency betw. the two) your suggestion here re: the 'cleaner' way to approach this is actually much more appropriate.

Thanks again for the contribution!

Owner

sbohlen commented Jun 1, 2014

Actually, scratch that -- I'll actually do this myself since its relatively straightforward to accomplish in short order....

@sbohlen sbohlen merged commit fdef6b8 into net-commons:master Jun 1, 2014

Owner

sbohlen commented Jun 1, 2014

Just FYI, since its the Common.Logging.Portable project that forms the basis for the Silverlight support and ConsoleOutLogger only exists in Common.Logging, I was able to remove all of the IFDEFs from the PR before merging it. Simplicity achieved!

@adamralph adamralph deleted the adamralph:colored-console-logging branch Jun 1, 2014

Contributor

adamralph commented Jun 1, 2014

😂 thanks!

Owner

sbohlen commented Jun 1, 2014

Sorry, just realized that I'm wrong -- there's been so much restructuring of the code here lately that I failed to realize that the ConsoleOutLogger does in fact also appear in the Silverlight project so I've added your IFDEFs back in -- so much for simplicty :-/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment