Add NoOpFactory implicit in package object

[armanbilge]

This is the no-op equivalent of:

log4cats/slf4j/src/main/scala/org/typelevel/log4cats/slf4j/package.scala

Line 23 in 4f02ef3

implicit def loggerFactoryforSync[F[_]: Sync]: Slf4jFactory[F] = new Slf4jFactory[F] {

[danicheg]

We intentionally didn't add an implicit method when adding NoOpFactory, see. I personally beware of this kind of thing since it accidentally may hurt someone in $PROD. Especially considering that some $IDE can suggest import when implicit value is required. But I can be overthinking about this. So, if the majority will be happy with this implicit method, I will be happy too.

[armanbilge]

Especially considering that some $IDE can suggest import when implicit value is required.

If this is so dangerous then I argue that LoggerFactory actually should not be an implicit at all. Since there can be multiple implementations, doesn't this go against our philosophy anyway?

[danicheg]

Since there can be multiple implementations, doesn't this go against our philosophy anyway?

Sorry, I don't quite follow your idea. I mean that if this method is just for tests, then it shouldn't be too hard to create an implicit instance on the user side. Or, probably, we can add this method straightforwardly to the testing module. Then it'd be a little safer from accidental bugs. But again, all of this could be just my imagination.

[armanbilge]

Ok, fair enough. My motivation is that currently there is no LoggerFactory for Scala.js. But maybe better to solve that problem directly.

• Implement a Scala.js ConsoleLogger #680

We will have a bigger problem for Scala Native, because there is no standard logging framework there.

[armanbilge]

Or, probably, we can add this method straightforwardly to the testing module.

To be fair, it's already isolated to the no-op module. If no-op was part of core that would be a bigger problem, but seems just as hard to add a no-op dependency as it is to add testing dependency :)

[danicheg]

If no-op was part of core that would be a bigger problem, but seems just as hard to add a no-op dependency as it is to add testing dependency :)

Yeah, true. Probably initial authors considering this fine. So I'm speaking just as a random guy from the community 🙇🏻‍♂️

[lorandszakacs]

LGTM. The implicit LoggerFactory for a Sync in slf4j module is way more dangerous :D and if we live with that we can at least be consistent about it.

[armanbilge]

@lorandszakacs actually I'm considering to retract this PR 😅

After some discussion I think it is wrong for LoggerFactory to be passed around implicitly, as I mentioned in #681 (comment).

question: should LoggerFactory be an implicit or explicit parameter? Seeing as there are multiple implementations (slf4j, noop, maybe something from otel4s in the future) seems to go against our usual philosophy for implicits?

oh and this PR would throw one more source of LoggerFactory into the mix 🙂

• Helpers for creating simple/stub versions of SelfAwareStructuredLogger and LoggerFactory #682

To which @ChristopherDavenport replies:

Multiple implementations is explicit.

https://discord.com/channels/632277896739946517/839254871646404638/1011694989870366770

Instead, I think we should deprecate and discourage implicit use of the Sl4fj factory.

[lorandszakacs]

I also lean towards deprecating the implicit creation of the factory in slf4j, in which case this indeed should be closed 😄

But I do not agree with encouraging people to not pass LoggerFactory implicitly, only with its automagic implicit creation. At the end of the day how many users will have more than one instance of a LoggerFactory in their codebase? And if they do, there always various ways of newtype-ing, or subclassing it, etc. to solve the problem. For large codebases with literally hundreds of classes 90% of which do logging users will probably pass these around implicitly.

[bplommer]

Why not provide this as a convenience but in an implicits namespace, like we do with (for example) the cats-effect global runtime? That way it's not just "floating around out there" but it's available in a convenient form when needed.

[lorandszakacs]

@armanbilge Shall we close this PR, and see if @bplommer's idea lands either in #683 or a follow-up?

[armanbilge]

Ah yes, I think we're all agreed we don't want to do this.