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

Allow comma separated values (List<T>) for Layout Renderers/Targets/Layouts #1439

Merged
merged 1 commit into from
Nov 23, 2016

Conversation

304NotModified
Copy link
Member

@304NotModified 304NotModified commented May 7, 2016

Support voor comma separated list,

e.g.

supported:

  • Generic List (List<T>), with the following types:
    • C# built in types (string, int, double, object)
    • enum (use shortname)
    • culture, encoding, Type
    • not supported: Layout

Not supported:

  • Arrays
  • Non-generic List
  • IList<T> / IList
  • IEnumerable<T> / IEnumerable

UPDATE support added for

Example:

${layoutrenderer-with-list:enums=Ignore, Neutral, ignore}
[LayoutRenderer("layoutrenderer-with-list")]
public class LayoutRendererWithListParam : LayoutRenderer
{
    public List<FilterResult> Enums { get; set; }
   protected override void Append(StringBuilder builder, LogEventInfo logEvent)
   {
       ...
   }
}

TO DO

  • escape comma -> backtick
  • replace escape with optional quoted
  • add unit test for multiple enum

This change is Reviewable

{
if (!resultType.IsEnum)
{
result = null;
return false;
}

if (resultType.IsDefined(typeof(FlagsAttribute), false))
if (flagsEnumAllowed && resultType.IsDefined(typeof(FlagsAttribute), false))
Copy link
Contributor

@bhaeussermann bhaeussermann May 10, 2016

Choose a reason for hiding this comment

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

What if it's a flags-enum, but flagsEnumAllowed is false? The else-part would then execute with the flags-enum.
Must it not just return false in this case?

Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding a test for parsing a list of values of a flag-enum?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea

@bhaeussermann
Copy link
Contributor

Any idea why the test coverage dropped so dramatically? I don't seem to be able to view the codecov diff.

@304NotModified
Copy link
Member Author

Any idea why the test coverage dropped so dramatically? I don't seem to be able to view the codecov diff.

It did not. 75.30% (+<.01%) compared to 717373a

but this PR has "only" 74.29% coverage

@304NotModified
Copy link
Member Author

304NotModified commented Jul 23, 2016

I will remove the ecape and will move to a optional quoted approach,

eg. a,b,c or 'a,b',c

Have to implemented that

# supported:

- Generic List (List<T>), with the following types:
 - C# built in types (string, int, double, object)
 - enum (use shortname)
 - culture, encoding, Type
 - not supported: Layout

# Not supported:

- Arrays
- Non-generic List
- IList<T> / IList (performance)
- IEnumerable<T> / IEnumerable  (performance)
@codecov-io
Copy link

Current coverage is 81% (diff: 96%)

Merging #1439 into master will increase coverage by <1%

@@             master      #1439   diff @@
==========================================
  Files           277        278     +1   
  Lines         17780      18230   +450   
  Methods        2785       2823    +38   
  Messages          0          0          
  Branches       2024       2098    +74   
==========================================
+ Hits          14332      14748   +416   
- Misses         2996       3006    +10   
- Partials        452        476    +24   

Sunburst

Powered by Codecov. Last update f900e35...61b4a72

@304NotModified 304NotModified merged commit 731d2f4 into master Nov 23, 2016
@304NotModified 304NotModified deleted the list-from-comma-separated-string branch November 23, 2016 19:52
@304NotModified 304NotModified changed the title Allow comma separated values (List) for Layout Renderers Allow comma separated values (List) for Layout Renderers/Targets/Layouts Nov 23, 2016
@304NotModified 304NotModified changed the title Allow comma separated values (List) for Layout Renderers/Targets/Layouts Allow comma separated values (List<T>) for Layout Renderers/Targets/Layouts Nov 23, 2016
@304NotModified
Copy link
Member Author

@304NotModified 304NotModified added the documentation done all docs done (wiki, api docs, lists on nlog-project.org, xmldocs) label Dec 4, 2016
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