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

${level} add format option (full, single char and ordinal) #2034

Merged
merged 5 commits into from Mar 30, 2017
Merged

${level} add format option (full, single char and ordinal) #2034

merged 5 commits into from Mar 30, 2017

Conversation

c0shea
Copy link
Contributor

@c0shea c0shea commented Mar 29, 2017

Adds single character and ordinal level output options to the LevelLayoutRenderer to address #1949 and #1950

@codecov-io
Copy link

codecov-io commented Mar 29, 2017

Codecov Report

Merging #2034 into master will increase coverage by <1%.
The diff coverage is 88%.

@@           Coverage Diff           @@
##           master   #2034    +/-   ##
=======================================
+ Coverage      82%     82%   +<1%     
=======================================
  Files         286     286            
  Lines       19663   19670     +7     
  Branches     2309    2310     +1     
=======================================
+ Hits        16042   16048     +6     
  Misses       3056    3056            
- Partials      565     566     +1

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 7c1d3a4...0d100a3. Read the comment docs.

@304NotModified
Copy link
Member

cool! Thanks!

Could you place the enum in a new file and outside the class? Please check other enums for the naming style.

to sync the csproj files, see https://github.com/NLog/NLog/blob/master/CONTRIBUTING.md#sync-projects

@@ -46,13 +48,44 @@ namespace NLog.LayoutRenderers
public class LevelLayoutRenderer : LayoutRenderer
{
/// <summary>
/// A value indicating when the cache is cleared.
Copy link
Member

Choose a reason for hiding this comment

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

I think this is wrong docs?

[Flags]
public enum LevelOutputOption
{
/// <summary>Ouput the name as the level.</summary>
Copy link
Member

Choose a reason for hiding this comment

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

Proposal:

  • render the full level name
  • render the first character for the level
  • render the ordinal (aka number) for the level.

/// Gets or sets a value indicating how the level is output.
/// </summary>
[DefaultValue(LevelOutputOption.Name)]
public LevelOutputOption LevelOutput { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking of "format", what do you think?

logger.Error("a");
AssertDebugLastMessage("debug", "E a");
logger.Fatal("a");
AssertDebugLastMessage("debug", "F a");
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! please add "trace"

logger.Error("a");
AssertDebugLastMessage("debug", "4 a");
logger.Fatal("a");
AssertDebugLastMessage("debug", "5 a");
Copy link
Member

Choose a reason for hiding this comment

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

also please add trace

@c0shea
Copy link
Contributor Author

c0shea commented Mar 30, 2017

You're welcome :)

I like your suggestion to name it Format, so I renamed the enum to LevelFormat and moved it to its own file. I modeled it after StackTraceFormat, so hopefully it follows the style. I also removed the wrong doc and updated the unit tests to add trace.

Let me know if you need anything else to accept.

@304NotModified 304NotModified self-assigned this Mar 30, 2017
@304NotModified
Copy link
Member

304NotModified commented Mar 30, 2017

Looks ready to merge! Could you please update the docs? Thanks!

Just add it to the wiki, with "introduced in 4.4.6". Maybe we should add the table with Short name and ordinal.

@304NotModified 304NotModified added this to the 4.4.6 milestone Mar 30, 2017
@304NotModified 304NotModified changed the title Level options ${level} add format option (full, single char and ordinal) Mar 30, 2017
@304NotModified
Copy link
Member

Fixes #1949 and fixes #1950

@304NotModified
Copy link
Member

docs see #2043

Will merge this an release a beta

@304NotModified 304NotModified merged commit ca5f70a into NLog:master Mar 30, 2017
@c0shea c0shea deleted the level-options branch March 30, 2017 22:34
@c0shea
Copy link
Contributor Author

c0shea commented Mar 30, 2017

Thanks! I have updated the docs with the new parameter and added a condensed table.

@304NotModified 304NotModified added the documentation done all docs done (wiki, api docs, lists on nlog-project.org, xmldocs) label Apr 15, 2017
@304NotModified
Copy link
Member

FYI,

NLog 4.4.6 is live!

https://www.nuget.org/packages/NLog/4.4.6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation done all docs done (wiki, api docs, lists on nlog-project.org, xmldocs) feature needs documentation on wiki
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants