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

Default ILogger implementations (netstandard 2.1) #1435

Merged
merged 8 commits into from
May 26, 2020

Conversation

skomis-mm
Copy link
Contributor

@skomis-mm skomis-mm commented May 19, 2020

What issue does this PR address?

Does this PR introduce a breaking change?
No.

Please check if the PR fulfills these requirements

  • The commit follows our guidelines
  • Unit Tests for the changes have been added (for bug fixes / features)

Other information:

  • Default ILogger implementation will make user story easier to implement the interface; with ability to further interface evolve. 72 overloads mirrors Logger methods: added test that checks the sync. For remaining methods added independent tests.

  • netstandard2.1 allows adding further improvements utilizing System.Span<>, System.Buffers etc without introducing new dependencies.

Additional cleanup, fixes, improvements, such as:

  • Fixed LoggerShouldNotReferenceToItsConfigurationAfterBeingCreated test that is failed under Debug;
  • Build / performance scripts actualization;
  • Added / changed tests tfms to LTS targets (should be easier to grab and build for those that have default VS setup);
  • Added .NET Framework reference packages to be able to build under Linux environment (checked in VSCode with WSL)

Sergey Komisarchik added 2 commits May 20, 2020 01:09
along with:

- build scripts actualization
- added latest LTS .net framework / core targets
- misc cleanup
@skomis-mm skomis-mm changed the title Default ILogger implemenations (netstandard 2.1) Default ILogger implementations (netstandard 2.1) May 19, 2020

[Theory]
[MemberData(nameof(DefaultInterfaceMethods))]
public void ILoggerDefaultMethodsShouldBeInSyncWithLogger(MethodInfo defaultInterfaceMethod)
Copy link
Member

Choose a reason for hiding this comment

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

Brilliant! Wouldn't have thought of this 😎

@nblumhardt
Copy link
Member

Looks awesome 👍

I'm wondering about the selection of non-default methods:

        public bool BindMessageTemplate(string messageTemplate, object[] propertyValues, out MessageTemplate parsedTemplate, out IEnumerable<LogEventProperty> boundProperties) => _inner.BindMessageTemplate(messageTemplate, propertyValues, out parsedTemplate, out boundProperties);
        public bool BindProperty(string propertyName, object value, bool destructureObjects, out LogEventProperty property) => _inner.BindProperty(propertyName, value, destructureObjects, out property);
        public ILogger ForContext(ILogEventEnricher enricher) => _inner.ForContext(enricher);
        public ILogger ForContext(string propertyName, object value, bool destructureObjects = false) => _inner.ForContext(propertyName, value, destructureObjects);
        public bool IsEnabled(LogEventLevel level) => _inner.IsEnabled(level);
        public void Write(LogEvent logEvent) => _inner.Write(logEvent);
        public void Write(LogEventLevel level, Exception exception, string messageTemplate, params object[] propertyValues) => _inner.Write(level, exception, messageTemplate, propertyValues);

I think these are the methods that any wrapper or non-trivial ILogger should probably implement explicitly 👍

For simple cases it seems like we could get away with Write(LogEvent) being the only requirement, and:

  • BindMessageTemplate(), BindProperty() - use a default implementation analogous to new LoggerConfiguration().CreateLogger() and use it to implement these
  • ForContext(...) - return a new ILogger implementation that wraps this and applies enrichment
  • IsEnabled() - return true
  • Write(LogEventLevel, Exception, ...) - implement in terms of the default BindMessageTemplate() etc.

On the one hand, this would make it slightly harder to write a useful delegating logger/wrapper (the user would need to discover the requirement to forward methods other than Write(LogEvent), but, it would increase the chances of a user successfully implementing an ILogger that has no Serilog Logger on the other side...

I think the choices in the PR are the safer ones, just keen to know what you think about the trade-off.

@skomis-mm
Copy link
Contributor Author

@nblumhardt totally makes sense 👍
Initially I provided methods with defaults that are not dependent on the state of the Loggers methods.

I just realized that was the wrong reason, since ILogger itself is pure abstraction, without implementation rules applied to it, so any reasonable implementation should be correct by default.

I will adjust the PR with your suggestions and will add custom tests for the remaining methods

Build.ps1 Outdated Show resolved Hide resolved
src/Serilog/ILogger.cs Outdated Show resolved Hide resolved
@nblumhardt
Copy link
Member

Looks good! Think it would be interesting to get this out there. Bumped the minor version to 2.10.

I missed the potential to call this.BindMessageTemplate() in Write() on my first pass - seems like that should probably be the flow, there.

@nblumhardt nblumhardt merged commit 8f2263c into serilog:dev May 26, 2020
@skomis-mm skomis-mm deleted the defaultIntf branch May 27, 2020 13:07
@nblumhardt nblumhardt mentioned this pull request Sep 10, 2020
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.

None yet

3 participants