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

Support for static member as the value of setting using :: #73 #77

Merged
merged 10 commits into from
Dec 15, 2017

Conversation

MrMon3y
Copy link
Contributor

@MrMon3y MrMon3y commented Dec 13, 2017

@tsimbalar

Support "Serilog.Sinks.SystemConsole.Themes.AnsiConsoleTheme::Grayscale" to access a static member for arguments of a complex type.

Solves issue #73

@tsimbalar
Copy link
Member

This looks pretty good 👍

Just a few remarks :

  • I believe all the classes under the *.TestDummies.* are not used anywhere, so they could be removed
  • to "prove" the correct behavior, it would be nice to update the Sample project so that it uses the Console sink (instead of Literate Console) and a custom theme with the new :: syntax.

@MrMon3y
Copy link
Contributor Author

MrMon3y commented Dec 13, 2017

@tsimbalar

PR Update

  • Removed unused TestDummies
  • Updated Sample project to use the Console sink and added a custom theme with new :: syntax.

Should this also be reflected in the readme.md ?

@@ -16,6 +16,7 @@
<ItemGroup>
<PackageReference Include="Microsoft.Extensions.Configuration.Json" Version="1.1.1" />
<PackageReference Include="Serilog.Sinks.Async" Version="1.0.1" />
<PackageReference Include="Serilog.Sinks.Console" Version="3.1.1" />
<PackageReference Include="Serilog.Sinks.Literate" Version="2.0.0" />
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 the package Serilog.Sinks.Literate is no longer required, because the sample app only use the Console Sink

@@ -0,0 +1,22 @@
namespace Serilog.Settings.Configuration.Tests.Support
{
public abstract class DummyAbstractClass
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 class (and its subclasses) are no longer needed. If this is the case, the file can probably be removed :)

@tsimbalar
Copy link
Member

Looks good !
I've added a few more comments, but I think this is close to being done 👍

Regarding the edit to the README.md, I think it will sooner or later be included as part of serilog/serilog#1053 so that won't be necessary for now.

Maybe a comment in the README.md of Serilog.Sinks.Console would be nice though, but I'll create a separate issue for that one.

@MrMon3y
Copy link
Contributor Author

MrMon3y commented Dec 14, 2017

@tsimbalar

PR Update

  • Removed stale PackageReference
  • Removed stale Class

@tsimbalar
Copy link
Member

Thanks Douglas, this looks good to me ! 👍
@nblumhardt @skomis-mm : shall I merge it ?

@nblumhardt
Copy link
Member

Looks good to me! I see we are really overdue for unification of the various configuration systems - quite a bit of duplication here, but 👍 for now.

@skomis-mm
Copy link
Contributor

Looks good! 👍

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

Successfully merging this pull request may close these issues.

5 participants