Add ability to render Headers

[petemounce]

No description provided.

[codecov-io]

Current coverage is 47.26%

Merging #27 into master will increase coverage by +0.79% as of 2be8def

@@            master     #27   diff @@
======================================
  Files           14      14       
  Stmts          142     146     +4
  Branches        29      31     +2
  Methods          0       0       
======================================
+ Hit             66      69     +3
- Partial          6       7     +1
  Missed          70      70       

Review entire Coverage Diff as of 2be8def

---

Uncovered Suggestions

1. +4.10% via ...ringTargetWrapper.cs#194...199
2. +3.42% via ...ringTargetWrapper.cs#166...170
3. +3.42% via ...ringTargetWrapper.cs#88...92
4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

[304NotModified]

Sorry, made a merge conflict because of another waiting PR (#22), which improved the coverage much!

[304NotModified]

Thanks for this PR. I looks good. We can merge this after fix of the conflict, BUT...

In #21 is discussed to move this to a separate layout renderer, e.g. ${aspnet-request-header}. What do you think about that? It's maybe a bit inconsistent with the current code, but aspnet-request:header=a:Item=b also is a bit odd (unclear what the order is and no influence on it). What do you think?

[petemounce]

@304NotModified I've rebased and adjusted the tests to use that introduced by #22.

I don't know about moving to a separate layout render for this - System.Web's days are numbered in the face of OWIN and ASP.NET vNext cutting loose. I think if a new layout renderer is introduced, it would be a better direction to introduce one that can reach into the OWIN environment for information to render into the log. While we're on that - are you aware of pysco68/Pysco68.Owin.Logging.NLogAdapter? Hi @pysco68!

(I should have said originally - I agree that the interface/config is a little unclear. But I still think the effort to fix this should instead be spent on a really good OWIN story for the same feature instead)

[304NotModified]

OK thanks!

No I'm not aware of owin for this. No experience with that. Is that something we should implement?

[304NotModified]

Maybe dumb question, but are owin and aspnet5 related?

[petemounce]

@304NotModified aspnet5 is built on top of OWIN. OWIN is basically an abstraction layer that allows a .NET website to be separated from the hosting - so it becomes much easier to host in, for example, one's own process (console app, windows service), in-memory (tests), or (still) IIS via a System.Web Adaptor. MS WebApi already supports OWIN, ASPNET MVC will do in the next release, and most of the non-MS OSS frameworks also already do.

OWIN gives a set of interfaces for requests, responses, and then also the notion that http is done via a chain-of-responsibility pipeline, which allows different middlewares to be plugged in that are agnostic of the web framework (Nancy, WebApi, soon MVC, etc) - this makes cross cutting concerns much easier.

[petemounce]

thanks for the merge :)

[petemounce]

IMO yes, OWIN is definitely something that it would be great to have NLog support.

[304NotModified]

Thanks for the info @petemounce !

About OWIN, yes it's nice although I have no problems to keep it external. Creating 2 times the same thing, that hasn't my preference. If the author (@pysco68) likes to move it to an official NLog repos, that would be also possible. (but we have to discuss how about license, ownership, maintenance etc).

Will publish 4.2 when I improved and merged #28

[pysco68]

Hello folks,

I didn't really understand the how my NLogAdapter got into this PR but anyhow I read the notification. I'm willing to discuss any option. As far as the code is considered, it's under MIT license so you can do pretty much whatever you want.

Just tell me when and where you'd like to discuss that, I believe that this PR isn't the right place for that however.

[304NotModified]

@petemounce I released 4.2 with this change. #28 will be 4.3 (include coreCLR)

@pysco68 sorry about the confusion.

[petemounce]

Thanks!

Sent from my phone. Please excuse typos and brevity, but never text speak.
On 7 Feb 2016 23:21, "Julian Verdurmen" [email protected] wrote:

@petemounce https://github.com/petemounce I released 4.2 with this
change. #28 #28 will be 4.3
(include coreCLR)

—
Reply to this email directly or view it on GitHub
#27 (comment).