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

Fix IHttpContextAccessor warning #180

Merged
merged 1 commit into from
Sep 29, 2017

Conversation

304NotModified
Copy link
Member

fixes #157

@codecov
Copy link

codecov bot commented Sep 29, 2017

Codecov Report

Merging #180 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #180   +/-   ##
=======================================
  Coverage   58.18%   58.18%           
=======================================
  Files          29       29           
  Lines         385      385           
  Branches       93       93           
=======================================
  Hits          224      224           
  Misses        125      125           
  Partials       36       36
Impacted Files Coverage Δ
...etCore/LayoutRenderers/AspNetLayoutRendererBase.cs 87.5% <ø> (ø) ⬆️

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 fc7c576...2337305. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 29, 2017

Codecov Report

Merging #180 into master will decrease coverage by 0.51%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #180      +/-   ##
==========================================
- Coverage   58.18%   57.66%   -0.52%     
==========================================
  Files          29       29              
  Lines         385      385              
  Branches       93       93              
==========================================
- Hits          224      222       -2     
  Misses        125      125              
- Partials       36       38       +2
Impacted Files Coverage Δ
...etCore/LayoutRenderers/AspNetLayoutRendererBase.cs 87.5% <ø> (ø) ⬆️
...youtRenderers/AspNetRequestCookieLayoutRenderer.cs 80.76% <0%> (-3.85%) ⬇️
...outRenderers/AspNetLayoutMultiValueRendererBase.cs 95.12% <0%> (-2.44%) ⬇️

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 fc7c576...2337305. Read the comment docs.

@304NotModified 304NotModified added this to the NLog.Web.AspNetCore 4.5 milestone Sep 29, 2017
@304NotModified 304NotModified changed the title fix IHttpContextAccessor warning Fix IHttpContextAccessor warning Sep 29, 2017
@304NotModified 304NotModified merged commit 9454c9f into master Sep 29, 2017
@304NotModified 304NotModified deleted the warning-IHttpContextAccessor branch September 29, 2017 21:33
@snakefoot
Copy link
Contributor

snakefoot commented Oct 1, 2017

@304NotModified Could be nice if AspNetLayoutRendererBase.Initialize() generated a warning, when HttpContextAccessor is null (And not a debug message for every property-access). As the initial proposal suggested #133 (but changed after some refactoring).

@304NotModified
Copy link
Member Author

@304NotModified Could be nice if AspNetLayoutRendererBase.Initialize() generated a warning, when HttpContextAccessor is null (

That was the case and we got multiple tickets for the warning. So now debug and only on usage.

@snakefoot
Copy link
Contributor

snakefoot commented Oct 1, 2017

Did you notice comments like these:

All logging fields like aspnet-* are not included in logs.

Which actually means the logging is not working, because it has been initialized wrong. Maybe the NLog Example should be fixed to perform proper initialization so the layout-renderer works.

But anyway just curious, why it has been removed, besides noise from people finding NLog hard to use in ASP.NET (Maybe fix the issue with it being hard to use)

@304NotModified
Copy link
Member Author

The new way (working on it), is less difficult to setup :)

@Surge001
Copy link

Surge001 commented Oct 4, 2017

Can't wait on the new way to be released. Please let us know in this trail when released!
thnx!

@304NotModified
Copy link
Member Author

Yes I will! Hopefully this weekend!

@snakefoot snakefoot added ASP.NET Core ASP.NET Core - all versions and removed ASP.NET Core 1 labels Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ASP.NET Core ASP.NET Core - all versions enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting-started-with-ASP.NET-Core vs2017
3 participants