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

Max message length option for Eventlog target #1127

Conversation

UgurAldanmaz
Copy link
Contributor

For #1102

@codecov-io
Copy link

Current coverage is 73.36%

Merging #1127 into master will increase coverage by +0.14% as of 29c05d4

@@            master   #1127   diff @@
======================================
  Files          264     264       
  Stmts        14923   14951    +28
  Branches      1633    1637     +4
  Methods          0       0       
======================================
+ Hit          10928   10969    +41
+ Partial        417     416     -1
+ Missed        3578    3566    -12

Review entire Coverage Diff as of 29c05d4


Uncovered Suggestions

  1. +0.09% via ...nternal/AspHelper.cs#206...218
  2. +0.09% via ...nternal/AspHelper.cs#188...200
  3. +0.09% via ...nternal/AspHelper.cs#170...182
  4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

@@ -131,8 +130,26 @@ public EventLogTarget(IAppDomain appDomain)
[DefaultValue("Application")]
public string Log { get; set; }

private int m_MaxMessageLength;
Copy link
Member

Choose a reason for hiding this comment

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

Please no Hungarian names. It's considered bad style.

@304NotModified
Copy link
Member

Thanks! Reviewed it, some small things.

Recommended read: http://dontcodetired.com/blog/post/Creating-Inline-Data-Driven-Tests-in-xUnit.aspx

Note: we are still on xunit 1 in this repos

@UgurAldanmaz
Copy link
Contributor Author

Thanks for review @304NotModified! I will be available to take a look them in detail this evening.

@304NotModified
Copy link
Member

👍

@UgurAldanmaz
Copy link
Contributor Author

I am done! Thanks again.

BTW, I made some comments, but they are not visible because of outdated diff. :)

@304NotModified
Copy link
Member

Thanks! But what about the argumentexpection? Is this consists with other targets? (I only see the removal of the unit test)

@304NotModified
Copy link
Member

Edit: see your comment now. Will read that first.

@304NotModified
Copy link
Member

I think you have a valid point. I will double check other targets for consistency, as I also don't know what is consistent now. :) (and I should have to know it)

@304NotModified
Copy link
Member

Oops, forgot to asign myself.

Well I don't like the ArgumentException, but it's consistent, so merge!

Great work! Thanks!

304NotModified added a commit that referenced this pull request Jan 15, 2016
…eventlog-target

Max message length option for Eventlog target
@304NotModified 304NotModified merged commit ff83d1a into NLog:master Jan 15, 2016
@304NotModified 304NotModified added this to the 4.3 milestone Jan 15, 2016
@304NotModified
Copy link
Member

@UgurAldanmaz Can you add the new option on the wiki? (please add "Introduced in NLog 4.3")

@UgurAldanmaz
Copy link
Contributor Author

I added wiki informations.

Thanks a lot @304NotModified!

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

Successfully merging this pull request may close these issues.

None yet

3 participants