Added ${aspnet-request-form} #313

DrewBrasher · 2018-09-07T12:43:19Z

No description provided.

… the request form collection.

…sp.net core and .net

…in the AspNetCore.Tests project.

codecov · 2018-09-07T12:47:02Z

Codecov Report

Merging #313 into master will increase coverage by 1%.
The diff coverage is 84%.

@@          Coverage Diff          @@
##           master   #313   +/-   ##
=====================================
+ Coverage      61%    62%   +1%     
=====================================
  Files          31     32    +1     
  Lines         435    454   +19     
  Branches       92     97    +5     
=====================================
+ Hits          266    282   +16     
- Misses        134    135    +1     
- Partials       35     37    +2

Impacted Files	Coverage Δ
...LayoutRenderers/AspNetRequestFormLayoutRenderer.cs	`84% <84%> (ø)`

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 9d95873...a842581. Read the comment docs.

304NotModified

Thanks!

Looks very good, added some notes for the final touches.

304NotModified · 2018-09-07T23:33:04Z

NLog.Web.AspNetCore/LayoutRenderers/AspNetRequestFormLayoutRenderer.cs

+        /// Gets or sets the form keys to exclude in the output.  If omitted, none are excluded.
+        /// </summary>
+        /// <docgen category='Rendering Options' order='10' />
+        public string Exclude { get; set; }


Idem, Set please :)

Sorry, I'm not sure what you are asking for on this one.

the ordering of the comments are changed. I meant: the same as #313 (comment)

304NotModified · 2018-09-07T23:33:41Z

NLog.Web.AspNetCore/LayoutRenderers/AspNetRequestFormLayoutRenderer.cs

+                    {
+                        formDataList.Add($"{item.Key}={item.Value}");
+                    }
+                }


Would be nice if we could reduce the code duplication here, (foreach / if check)

Removed code duplication by changing to a foreach that works for both AspNetCore and .net framework. https://github.com/DrewBrasher/NLog.Web/commit/8b7999a93bc75e3834e21d5cd999824bea184976

304NotModified · 2018-09-07T23:33:50Z

NLog.Web.AspNetCore/LayoutRenderers/AspNetRequestFormLayoutRenderer.cs

+        /// Gets or sets the form keys to include in the output.  If omitted, all are included.
+        /// </summary>
+        /// <docgen category='Rendering Options' order='10' />
+        public string Include { get; set; }


You could use list/set here :)

(Since NLog 4.4 - https://nlog-project.org/2016/12/14/nlog-4-4-is-live.html)

I prefer a ISet<String> (for .NET 3.5 is should be HashSet<String>)

You could make it default case insensitive.

Something like this?

#if ASP_NET_CORE public ISet<string> Include { get; set; } #else public HashSet<string> Include { get; set; } #endif

yes indeed 👍

Changed Include and Exclude to case insensitive HashSets.
https://github.com/DrewBrasher/NLog.Web/commit/8eb037049c212c70d2b85668883788d0565d5bb4

… AspNetCore and .net framework .

304NotModified · 2018-09-10T21:17:59Z

nice!

One question, if an item is in include and exclude, which one has precedence? It seems to be exclude, but i'm not sure if that's common. (we need to check that)

DrewBrasher · 2018-09-11T12:33:51Z

You are correct that exclude currently takes precedence. I've looked around and see examples of other software doing it both ways. Either way is fine with me and I will add a unit test to cover whichever way is decided.

Here are some examples:
Include takes precedence
https://docs.citrix.com/en-us/profile-management/current-release/configure/include-and-exclude-items.html

Exclude Takes precedence:
https://docs.microsoft.com/en-us/windows/deployment/usmt/usmt-conflicts-and-precedence

https://stackoverflow.com/questions/11959211/maven-surefire-plugin-include-exclude-precedence

Other examples:
https://gist.github.com/ento/cf5593d4fac3f72d3765b3fe2ed2a204

DrewBrasher · 2018-09-13T18:42:41Z

@304NotModified which one would you rather have take precedence?

304NotModified · 2018-09-15T19:37:23Z

HI,

Thanks for looking at this! I think docs are the most important and as it is inconsistent, I don't prefer really one above the other. So we could keep it like this.

304NotModified · 2018-09-15T19:37:53Z

will try to finish this PR in the beginning of next week

304NotModified

Thanks! looks good!

One small improvement is possible, see note

304NotModified · 2018-09-19T19:39:00Z

NLog.Web.AspNetCore/LayoutRenderers/AspNetRequestFormLayoutRenderer.cs

+        /// Gets or sets the separator to use between each key/value pair.  If omitted, <see cref="Environment.NewLine"/> is used.
+        /// </summary>
+        /// <docgen category='Rendering Options' order='10' />
+        public string Separator { get; set; }


Maybe this should be a Layout. What do you think? We can use then ${newline} etc

That would be better. I didn't realize I could do that. Do you think I should still have newline as the default separator or would something else be better as the default? The main reason I had newline as the default was because I didn't know I could set it up to take a layout. Thanks!

I think newline is fine, but in other renderers we use space. Not sure what would be the best in this case.

I will change it to a space to keep it consistent with the others. With it being a Layout instead of a string people can easily make it a newline if they want.

I just noticed the base class AspNetLayoutMultiValueRendererBase and I'm wondering if I should change this to extend that class.

doubting about that. It isn't a bad idea, but unfortunately no Layouts there. Both are fine to me.

PS I like to release this soon :)

fixed the layout thing (see #317), so please subclass from
AspNetLayoutMultiValueRendererBase

OK. I will do that now.

304NotModified · 2018-09-22T16:24:50Z

NLog.Web.AspNetCore/LayoutRenderers/AspNetRequestFormLayoutRenderer.cs

+        /// Gets or sets the separator to use between each key/value pair.  If omitted, <see cref="Environment.NewLine"/> is used.
+        /// </summary>
+        /// <docgen category='Rendering Options' order='10' />
+        public string Separator { get; set; }


fixed the layout thing (see #317), so please subclass from
AspNetLayoutMultiValueRendererBase

304NotModified · 2018-09-22T16:29:43Z

PS could you see these results?

DrewBrasher · 2018-09-22T16:34:59Z

It won't let me view the details: [image: image.png]

…

On Sat, Sep 22, 2018 at 11:29 AM Julian Verdurmen ***@***.***> wrote: PS could you see these results? [image: image] <https://user-images.githubusercontent.com/5808377/45919451-778e0200-be95-11e8-8477-1d69b3c45213.png> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#313 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AI194nR6DHVnqyY0FeNjaWEKjacGQ4TMks5udmV4gaJpZM4Wewq-> .

-- Thanks, *Drew Brasher* drewbrasher.com

304NotModified · 2018-09-22T16:38:04Z

Could you please try again? (added you as Collaborator)

(I think you have an email, with a link you should click first)

304NotModified · 2018-09-22T16:39:23Z

anyway for now:

…rm-layout-renderer

…alueRendererBase

DrewBrasher · 2018-09-22T21:00:03Z

I've changed AspNetRequestFormLayoutRenderer to be a subclass of AspNetLayoutMultiValueRendererBase and fixed the code check issue.

304NotModified · 2018-09-22T21:52:12Z

🎉 great!

304NotModified · 2018-09-22T21:53:32Z

Could you please help with docs on the wiki and https://github.com/NLog/NLog.github.io/blob/master/config/layout-renderers.json?

I will prepare a release :)

304NotModified · 2018-09-22T23:17:27Z

4.7 is live :)

Thanks for the help!

DrewBrasher · 2018-09-24T12:09:56Z

I added aspnet-request-form info to config/layout-renderers.json (https://github.com/DrewBrasher/NLog.github.io/commit/1aab227888d90d29644b6f42f9de76eaa4cd3f1d). What all has to be done to add to the documentation?

304NotModified · 2018-09-24T18:51:23Z

We need a page on the wiki, (and it's linked by https://github.com/DrewBrasher/NLog.github.io/commit/1aab227888d90d29644b6f42f9de76eaa4cd3f1d#diff-ca51f403fc41bbc57d0471b9a7f64a3fR1730) - so we need https://github.com/NLog/NLog/wiki/AspNetRequest-Form-Layout-Renderer

Thanks in advance!

Drew Brasher added 4 commits September 6, 2018 07:10

Added aspnet-request-form layout renderer that outputs all or some of…

6cd8f8b

… the request form collection.

Made the aspnet-request-form layout renderer output uniform in both a…

d179da2

…sp.net core and .net

Added unit tests for aspnet-request-form layout renderer.

be8f5bf

Fixed issues with the unit tests for aspnet-request-form not working …

bf57a36

…in the AspNetCore.Tests project.

304NotModified requested changes Sep 7, 2018

View reviewed changes

304NotModified added this to the NLog.Web + NLog.Web.AspNetC4.7 milestone Sep 7, 2018

Drew Brasher added 2 commits September 10, 2018 09:25

Removed code duplication by changing to a foreach that works for both…

8b7999a

… AspNetCore and .net framework .

Changed Include and Exclude to case insensitive HashSets.

8eb0370

304NotModified self-assigned this Sep 15, 2018

304NotModified added the feature label Sep 15, 2018

Added unit test to cove exclude/include precedence and updated comments.

729e1bc

304NotModified reviewed Sep 19, 2018

View reviewed changes

304NotModified changed the title ~~Add aspnet request form layout renderer~~ Added ${aspnet-request-form} Sep 22, 2018

304NotModified requested changes Sep 22, 2018

View reviewed changes

DrewBrasher added 2 commits September 22, 2018 11:49

Merge remote-tracking branch 'nlog/master' into add-aspnet-request-fo…

bccf894

…rm-layout-renderer

Made AspNetRequestFormLayoutRenderer a subclass of AspNetLayoutMultiV…

a842581

…alueRendererBase

304NotModified added docs needed needs documentation on nlog-project.org/config labels Sep 22, 2018

304NotModified approved these changes Sep 22, 2018

View reviewed changes

304NotModified merged commit 21989ad into NLog:master Sep 22, 2018

This was referenced Sep 22, 2018

Log all the form variables #194

Closed

docs ${aspnet-request-form} #319

Closed

snakefoot added docs docs done and removed needs documentation on nlog-project.org/config docs needed labels Jan 9, 2023

pull-request-size bot added the size/L label Jan 9, 2023

Added ${aspnet-request-form} #313

Added ${aspnet-request-form} #313

Conversation

DrewBrasher commented Sep 7, 2018

codecov bot commented Sep 7, 2018 • edited Loading

Codecov Report

304NotModified left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

304NotModified Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

304NotModified commented Sep 10, 2018 • edited Loading

DrewBrasher commented Sep 11, 2018

DrewBrasher commented Sep 13, 2018

304NotModified commented Sep 15, 2018

304NotModified commented Sep 15, 2018

304NotModified left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

304NotModified commented Sep 22, 2018

DrewBrasher commented Sep 22, 2018 via email

304NotModified commented Sep 22, 2018 • edited Loading

304NotModified commented Sep 22, 2018

DrewBrasher commented Sep 22, 2018

304NotModified commented Sep 22, 2018

304NotModified commented Sep 22, 2018

304NotModified commented Sep 22, 2018

DrewBrasher commented Sep 24, 2018

304NotModified commented Sep 24, 2018

codecov bot commented Sep 7, 2018 •

edited

Loading

304NotModified Sep 10, 2018 •

edited

Loading

304NotModified commented Sep 10, 2018 •

edited

Loading

304NotModified commented Sep 22, 2018 •

edited

Loading