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

BeginScope accepts any type of KeyValuePair and Dictionary #223

Merged
merged 1 commit into from
Jul 19, 2018

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented May 30, 2018

Make BeginScope more user-friendly by accepting any type of KeyValuePair.

There is an overhead from lookup in ConcurrentDictionary and boxing, so maybe one should just write to the InternalLogger instead of doing reflection.

@304NotModified
Copy link
Member

304NotModified commented May 31, 2018

AFAIK we could use the pattern matching with open generics (c# 7.1 - https://github.com/dotnet/csharplang/blob/master/proposals/csharp-7.1/generics-pattern-match.md).

That would preform better I think

update: not sure if that will work, we need to know T etc I think.

It's a bit pity KeyValuePair isn't implementing a covariant interface

@codecov
Copy link

codecov bot commented May 31, 2018

Codecov Report

Merging #223 into master will increase coverage by 0.17%.
The diff coverage is 71.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #223      +/-   ##
==========================================
+ Coverage   72.15%   72.32%   +0.17%     
==========================================
  Files           6        6              
  Lines         316      365      +49     
  Branches       75       89      +14     
==========================================
+ Hits          228      264      +36     
- Misses         59       65       +6     
- Partials       29       36       +7
Impacted Files Coverage Δ
src/NLog.Extensions.Logging/Logging/NLogLogger.cs 74.31% <71.69%> (-0.84%) ⬇️
...g.Extensions.Logging/Logging/NLogLoggerProvider.cs 94.28% <0%> (+2.85%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d0d78f...b340362. Read the comment docs.

@snakefoot
Copy link
Contributor Author

That would preform better I think

Changed the logic into using cached expression-trees, then the performance should not be super horrible.

@snakefoot
Copy link
Contributor Author

@304NotModified Please make one more review, when you get the time. Now one can also inject a single KeyValuePair instead of a whole list.

@304NotModified 304NotModified self-assigned this Jun 22, 2018
@304NotModified
Copy link
Member

Thanks, I'm on holiday but will try to find some time these days

@304NotModified 304NotModified added this to the 1.2.0 milestone Jul 19, 2018
@304NotModified 304NotModified changed the title BeginScope should accept any type of KeyValuePair BeginScope accepts any type of KeyValuePair and Dictionary Jul 19, 2018
@304NotModified 304NotModified merged commit 987a773 into NLog:master Jul 19, 2018
@304NotModified
Copy link
Member

merged! Thanks!

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.

2 participants