This is my attempt to fix issue #16 with Logging trait

[akorneev]

No description provided.

[lightbend-cla-validator]

Hi @akorneev,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Typesafe Contributors License Agreement:

http://www.typesafe.com/contribute/cla

[akorneev]

Ok, signed the CLA now.

[hseeberger]

@akorneev, thanks for your work, looks pretty good. Yet I think that instead of adding another abstraction, we should think about simplifying things: As there's only one implementation for the API module, namely the SLF4J based one, we could just drop the API module. What do you think?

[akorneev]

Actually, this separation between API and implementation was the only reason we decided to upgrade from 1.x branch.

First, even if we currently have no other implementations except SLF4J based one, it makes a code much cleaner, when abstractions depends only on another abstractions:

trait This { requires: That with Logging => //... } //usage class ThisImpl extends This with ThatImpl with slf4j.Logging

Second, it simplifies testing of logging code, because its much easier to mock Logging trait than than the SLF4J logger itself:

class ThisSpec extends FunSpec with MockitoSugar { //this will be used to test logging code trait LoggingMock extends Logging { class MockedLogger extends Logger { override def adapter: LoggerAdapter = mock[LoggerAdapter] } override def logger: Logger = new MockedLogger } //it's methods will do nothing trait NoopLogging extends Logging { //... } describe("This"){ it("should log something") { val test = new This extends ThatMock with LoggingMock test.doSomething() verify(test.logger.adapter).error("something") } it("should not make much noise in console") { val test = new This extends ThatMock with NoopLogging test.doSomething() } } }

[hseeberger]

I don't buy your first argument, I rather consider the logging abstraction over engineered, at least given the current state.

Wrt the second argument, do you really consider logging something worth mocking?

[akorneev]

Sometimes, yes. But I agree that the ability to mock in itself is hardly worth the additional complexity.

[akorneev]

Updated.

[hseeberger]

@akorneev, could you please describe the goal and design for the latest changes?

[akorneev]

Sure. I implemented your suggestion to drop the API module. I removed the Logging trait and moved the rest of scala-logging-api module into scala-logging-slf4j.

If someone wants to use scala-logging, he should add the scala-logging-slf4j dependency and mix in the LazyLogging or StrictLogging trait directly (instead of Logging, as it was earlier).

The project now has two modules: scala-logging-slf4j and scala-logging-slf4j-macro. The first contains what now becomes API for SLF4J-based logging, and the last contains the actual macro implementation. I don't like the separate module for macro, but it seems the simplest solution to make the macro work (this approach is officially recommended).

[hseeberger]

@akorneev, thank you!

I don't think it's necessary for a library to have separate projects, that's only needed for the library using macros and user code. Could you please give it a try?

What do others think about removing the API module?

[akorneev]

@hseeberger, your are right, we actually do not need separate project for macros, removed it.

And a small correction to my previous comment: I actually did not removed the Logging trait, just moved it into com.typesafe.scalalogging.slf4j package. So, if someone wants to use it just to declare logger member, it is there.

[hseeberger]

I have simplified Scala Logging substantially, e.g. only one module, one package, etc. Check out the new version 3.0.0. Hence this is no longer relevant, but nevertheless kudos for your contributions!