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

Make Set methods of MDC and MDLC return IDisposable #2592

Merged
merged 3 commits into from
Mar 7, 2018

Conversation

MikeFH
Copy link
Contributor

@MikeFH MikeFH commented Feb 22, 2018

Attempt at implementing #2526

@codecov
Copy link

codecov bot commented Feb 22, 2018

Codecov Report

Merging #2592 into master will decrease coverage by <1%.
The diff coverage is 79%.

@@           Coverage Diff           @@
##           master   #2592    +/-   ##
=======================================
- Coverage      81%     81%   -<1%     
=======================================
  Files         325     325            
  Lines       23918   23974    +56     
  Branches     3022    3026     +4     
=======================================
+ Hits        19408   19434    +26     
- Misses       3693    3728    +35     
+ Partials      817     812     -5

@304NotModified 304NotModified added the breaking change Breaking API change (different to semantic change) label Feb 22, 2018
@304NotModified
Copy link
Member

Cool! But unfortunately this is a breaking change.

So proposal to add a new method, Scope or something like that?

@MikeFH
Copy link
Contributor Author

MikeFH commented Feb 22, 2018

Got it.
I guess you can close this PR, I'll open a new one with another solution. I don't know when though, so if someone wants to look at it, it's cool.

@snakefoot
Copy link
Contributor

snakefoot commented Feb 22, 2018

You could also call the method Push or PushProperty (Leaving Set untouched, and making it a non-breaking change):

https://github.com/serilog/serilog/blob/3cd376c8c75ed49ab307c50cd80beaa06cbaecb7/src/Serilog/Context/LogContext.cs#L77

When doing the dispose, then it will Pop/Remove the property.

@304NotModified
Copy link
Member

guess you can close this PR, I'll open a new one with another solution

Just push new commits on your branch, we can squash them later.

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.

New methods. Name tba

Revert Set methods to their original forms
@MikeFH
Copy link
Contributor Author

MikeFH commented Feb 26, 2018

I added a new method. It is called Push as suggested, but maybe it is a bit misleading since it might imply that the user works on a stack with this method (like in serilog) and that's not what we have here.

@304NotModified
Copy link
Member

What do you think about SetScoped?

@MikeFH
Copy link
Contributor Author

MikeFH commented Feb 27, 2018

SetScoped it is.
I can still rename it if someone comes up with a better idea.

@MikeFH
Copy link
Contributor Author

MikeFH commented Feb 28, 2018

Build seems to be ok on my appveyor and travis accounts.

@304NotModified
Copy link
Member

Build seems to be ok on my appveyor and travis accounts.

Tests are sometime a bit unstable :(

Will restart the CI

@304NotModified 304NotModified self-assigned this Mar 5, 2018
@304NotModified 304NotModified added this to the 4.5 rc7 milestone Mar 7, 2018
@304NotModified 304NotModified removed the breaking change Breaking API change (different to semantic change) label Mar 7, 2018
@304NotModified
Copy link
Member

Looks great! merged!

Thanks!

@304NotModified 304NotModified merged commit e9da996 into NLog:master Mar 7, 2018
@snakefoot snakefoot mentioned this pull request Mar 15, 2018
@MikeFH MikeFH deleted the mdc_mdlc_set_disposable branch September 23, 2018 16:51
@snakefoot snakefoot modified the milestones: 4.5 rc7, 4.5 Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants