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

JsonLayout - Don't mark ThreadAgnostic when IncludeMdc or IncludeMdlc is enabled #2164

Merged
merged 1 commit into from
Jun 17, 2017

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Jun 12, 2017

Different solution to the same problem.

@snakefoot snakefoot force-pushed the JsonLayoutThreadAgnostic2 branch 4 times, most recently from 7ac0108 to 41ff0ca Compare June 13, 2017 17:12
@@ -63,7 +63,7 @@ public abstract class Layout : ISupportsInitialize, IRenderable
///
/// Thread-agnostic layouts only use contents of <see cref="LogEventInfo"/> for its output.
/// </remarks>
internal bool ThreadAgnostic { get; private set; }
internal bool ThreadAgnostic { get; set; }
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 to make it protected set. What do you think?

Copy link
Contributor Author

@snakefoot snakefoot Jun 15, 2017

Choose a reason for hiding this comment

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

Well internal is "almost" private so it is more restrictive than protected. You are not allowed to upgrade the setter to protected, so it can be accessed by external classes.

Else the entire property should become protected, and part of the official interface. I don't think this internal optimization detail, should be exposed.

Copy link
Member

Choose a reason for hiding this comment

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

I meant protected internal. Not sure if that could be set on the setter only.

Its indeed and internal optimization (just makes the intention more clear).

Copy link
Member

Choose a reason for hiding this comment

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

Scratch that, its then protected or internal. Not and

@304NotModified
Copy link
Member

304NotModified commented Jun 15, 2017

I really prefer this solution :)

(reported here: #2109 (comment))

@304NotModified
Copy link
Member

If you rebase on the latest master, does sonarqube work then?

@snakefoot
Copy link
Contributor Author

If you rebase on the latest master, does sonarqube work then?

Not that strong with GIT. Can only see one master-branch, and no changes has been made to this for some time, and this PR is branched of that same master. What do you mean?

@304NotModified
Copy link
Member

Could you do

git remote -v

@snakefoot
Copy link
Contributor Author

git remote -v

andre   https://github.com/AndreGleichner/NLog.git (fetch)
andre   https://github.com/AndreGleichner/NLog.git (push)
nlog    https://github.com/NLog/NLog/ (fetch)
nlog    https://github.com/NLog/NLog/ (push)
origin  https://github.com/snakefoot/NLog.git (fetch)
origin  https://github.com/snakefoot/NLog.git (push)

@304NotModified
Copy link
Member

304NotModified commented Jun 15, 2017

Just on mobile, so hopefully no errors

git fetch --all
git rebase nlog/master

After fixing stuff (git rebase --continue)

When done

git push --force-with-lease

@304NotModified
Copy link
Member

Updated it, you need fetch all

@snakefoot
Copy link
Contributor Author

@304NotModified Updated and triggered rebuild

@304NotModified 304NotModified added the enhancement Improvement on existing feature label Jun 16, 2017
@codecov
Copy link

codecov bot commented Jun 16, 2017

Codecov Report

Merging #2164 into master will increase coverage by <1%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #2164    +/-   ##
=======================================
+ Coverage      81%     81%   +<1%     
=======================================
  Files         291     291            
  Lines       20124   20135    +11     
  Branches     2392    2394     +2     
=======================================
+ Hits        16343   16367    +24     
  Misses       3152    3152            
+ Partials      629     616    -13

@304NotModified 304NotModified added this to the 4.4.11 milestone Jun 17, 2017
@304NotModified 304NotModified merged commit 04aacc1 into NLog:master Jun 17, 2017
@304NotModified 304NotModified changed the title JsonLayout - IncludeMdc and IncludeMdlc are not ThreadAgnostic v2 JsonLayout - Don't mark ThreadAgnostic when IncludeMdc or IncludeMdlc is enabled Jun 17, 2017
@snakefoot snakefoot deleted the JsonLayoutThreadAgnostic2 branch October 10, 2017 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement on existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants