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

Additional ASP.NET renderers (.NET Core compliant) & unit tests for .NET Core #54

Merged
merged 42 commits into from
Jul 8, 2016

Conversation

s-sreenath
Copy link
Contributor

@s-sreenath s-sreenath commented Apr 30, 2016

  • aspnet-request-cookie - To Render cookie(s) from the request. Allows comma separated to get multiple cookie values. Can also allow json formatting.

  • aspnet-request-querystring - querystring

  • aspnet-request-method - the method (GET/POST etc)

  • aspnet-request-referrer - To Render the Referrer URL.

  • aspnet-request-url - To Render the Request URL.

  • aspnet-useragent - To Capture the Useragent of the request.

  • aspnet-host- The host of the request

  • aspnet-mvc-action - action name

  • aspnet-mvc-controller - controller name

Related #53

aspnet-request-cookie Renderer
aspnet-request-referrer Renderer. The ASP.NET MVC CORE Version is not
yet tested.
aspnet-request-referrer fixed the ASP.NET MVC CORE.
Completed aspnet-request-url and did some improvements to the
aspnet-request-referrer.
aspnet-useragent
public class CookieTests
{
[Fact]
public void NullKeyRendersEmptyString()
Copy link
Member

Choose a reason for hiding this comment

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

Looks good, maybe it's nice to create some helper methods to prevent code duplication

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the code duplication you are referring here?

@304NotModified
Copy link
Member

I think it looks good, I added some notes but all of them are minor code style notes.

I prefer to keep the code between #if etc as small as possible, as this is better for maintainability.

@s-sreenath s-sreenath self-assigned this May 1, 2016
s-sreenath added 2 commits May 1, 2016 17:23
PR Comments and improvement to documentation.
aspnet-request-method
/// To control the Cooked Renderer Output formatting.
/// </summary>
public enum AspNetCookieLayoutOutPutFormat
{
Copy link
Member

Choose a reason for hiding this comment

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

OutPut => Output :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Fixed the Build Errors.
Fixed the Build Errors.
@304NotModified
Copy link
Member

304NotModified commented Jul 8, 2016

Looks good!

We have to do three things:

  1. 1. Update wiki
  2. 2. Create new NLog release (using prerelease here)
  3. 3. Enable unit tests for netcore on appveyor (appveyor.yml)

Correct?

@s-sreenath
Copy link
Contributor Author

s-sreenath commented Jul 8, 2016

1 Update wiki

How do you think this needs to be done? Any pointers would be helpful.

2 Create new NLog release (using prerelease here)

Yes, we would need a release which has List Renderer support.

3 Enable unit tests for netcore on appveyor (appveyor.yml)

Yes.

We would also need to add examples on how to use it. May be pointers back to Tests Cases for starters.

@304NotModified
Copy link
Member

I have merged it with the unit tests branch, so the unit tests should work now :)

1 Update wiki
How do you think this needs to be done? Any pointers would be helpful.

Add to https://github.com/NLog/NLog/wiki/Layout-Renderers and add the text "Introduced in NLog.Web 4.3". So new page for every layout renderer. Would be nice to add some docs to the "legacy" layout renderers.

2 Create new NLog release (using prerelease here)
Yes, we would need a release which has List Renderer support.

Can you check /review NLog/NLog#1439 for me? Im doubting especially with the current escape character: the backtick (`). To bad we cannot use backslash (because it's also an escape of the layout renderers already)

3 Enable unit tests for netcore on appveyor (appveyor.yml)
Yes.

This should be done now. Nice that this branch is in this repo :)

We would also need to add examples on how to use it. May be pointers back to Tests Cases for starters.

That would be nice indeed

@304NotModified
Copy link
Member

oops broke the unit tests on net full. Worked on my machine...

@304NotModified
Copy link
Member

304NotModified commented Jul 8, 2016

don't get it. Works on my machine

image

@304NotModified
Copy link
Member

sorry, broke it, working on fixing it. Merge was wrong

@304NotModified 304NotModified force-pushed the additional-renderers branch from da6996f to ee03151 Compare July 8, 2016 23:16
@304NotModified
Copy link
Member

should be fixed

@304NotModified 304NotModified modified the milestone: 4.3 Jul 8, 2016
@304NotModified 304NotModified merged commit eb9ea06 into master Jul 8, 2016
@304NotModified
Copy link
Member

merged :)

@s-sreenath
Copy link
Contributor Author

Documentation completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants