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

ConsoleTarget - Apply Encoding on InitializeTarget, if Console available #1850

Merged
merged 4 commits into from
Dec 18, 2016

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Dec 13, 2016

Attempt to fix #1845 using detectConsoleAvailable=true


This change is Reviewable

@snakefoot snakefoot force-pushed the ConsoleTargetEncoding branch 2 times, most recently from 6932272 to ee16185 Compare December 13, 2016 22:03
@codecov-io
Copy link

codecov-io commented Dec 13, 2016

Current coverage is 82% (diff: 62%)

Merging #1850 into master will increase coverage by <1%

@@             master      #1850   diff @@
==========================================
  Files           276        276          
  Lines         18472      18503    +31   
  Methods        2857       2859     +2   
  Messages          0          0          
  Branches       2129       2137     +8   
==========================================
+ Hits          15061      15096    +35   
+ Misses         2922       2915     -7   
- Partials        489        492     +3   

Sunburst

Powered by Codecov. Last update fa1eb2d...1751df7

Copy link
Member

@304NotModified 304NotModified left a comment

Choose a reason for hiding this comment

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

any idea if we could unit test this?

@@ -86,17 +86,23 @@ public sealed class ConsoleTarget : TargetWithLayoutHeaderAndFooter
[DefaultValue(false)]
public bool Error { get; set; }

#if !SILVERLIGHT && !__IOS__ && !__ANDROID__
/// <summary>
/// The encoding for writing messages to the <see cref="Console"/>.
/// </summary>
/// <remarks>Has side effect</remarks>
public Encoding Encoding
Copy link
Member

Choose a reason for hiding this comment

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

In SILVERLIGHT / IOS / ANDROID we have now the property Encoding, but it doesn't work. Do you think that's an improvement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no opinion. Just removed the defines, since the property was no longer using the console directly.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer removing it, as it won't do anything. This could be confusing from the API.

@304NotModified 304NotModified added bug Bug report / Bug fix enhancement Improvement on existing feature labels Dec 14, 2016
@snakefoot
Copy link
Contributor Author

snakefoot commented Dec 14, 2016

I guess one could try and test if the set encoding actually updates the Console.OutputEncoding, but I can see you added the encoding property without any tests :)

@snakefoot snakefoot force-pushed the ConsoleTargetEncoding branch 3 times, most recently from b6567bc to 7a7e2c0 Compare December 14, 2016 22:23
@snakefoot
Copy link
Contributor Author

Made a unit-test, and found a bug when dynamically changing ConsoleTarget.Encoding after it has been initialized. Guess I learned my lesson once again :)

@@ -86,17 +86,23 @@ public sealed class ConsoleTarget : TargetWithLayoutHeaderAndFooter
[DefaultValue(false)]
public bool Error { get; set; }

#if !SILVERLIGHT && !__IOS__ && !__ANDROID__
/// <summary>
/// The encoding for writing messages to the <see cref="Console"/>.
/// </summary>
/// <remarks>Has side effect</remarks>
public Encoding Encoding
Copy link
Member

Choose a reason for hiding this comment

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

I prefer removing it, as it won't do anything. This could be confusing from the API.

@304NotModified
Copy link
Member

Looks good, only the API change is a breaker (for SILVERLIGHT /IOS /ANDROID).

@304NotModified 304NotModified added this to the 4.4.1 milestone Dec 18, 2016
@304NotModified
Copy link
Member

Thanks!

@304NotModified 304NotModified merged commit 882e895 into NLog:master Dec 18, 2016
@snakefoot snakefoot deleted the ConsoleTargetEncoding branch October 10, 2017 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug report / Bug fix console-target enhancement Improvement on existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants