jokade / slogging

A Typesafe-logging (and slf4j) compatible logging library based on macros for Scala/JVM, Scala.js, and Scala Native
MIT License
50 stars 11 forks source link

Optional params not working in ConsoleLoggerFactory #16

Open chandu0101 opened 8 years ago

chandu0101 commented 8 years ago

val obj = json(x = 5)
logger.info(s"obj 1",obj) // not printing obj in console
dom.window.console.log(s"obj 2 ",obj) // working fine
jokade commented 8 years ago

That's a misunderstanding - due to missing documentation: slogging implements the SLF4J API, i.e. additional arguments to the logging methods are not simply appended to the message, but are used to replace {} in the provided message. So if you want to log an object, you need to use something like

val obj = literal(x = 5)
logger.info("obj: {}",obj)

However, this was not properly implemented in previous versions of slogging, so you need to use 0.5.0 for this to work.

pjazdzewski1990 commented 7 years ago

To "piggyback" on the question above.

I think there's more than just the API. As I understand, the SLF4J convention suggests merging varargs and the logging statement by calling the toString method on every element replacing {}. I'm not a js expert, but toString from JavaScript is not a very useful method. To give an example: const foo = {"foo": "bar"}; console.log("Message was: " + foo.toString() + "!"); console.log("Message was: !", foo); The first output is "Message was: [object Object]!" which is not very practical as we can't peek into the instance. The second approach has more browser support thus is easier to use.

Any suggestions on how to walk around the issue? In theory you might pass many additional arguments to console.log so maybe ConsoleLoggerFactory should be modified to take advantage of it?

jokade commented 7 years ago

@pjazdzewski1990 I agree that the current handling of JS objects is unsatisfactory. However, the main goal of this library is to provide consistent logging for JVM, JS, and Scala Native.

Let's consider your example: we could change ConsoleLogger to emit

console.log("Message was: ",foo);

when calling

logger.info("Message was:", foo)

That would work fine when we use ConsoleLoggerFactory. But when the user changes the logger factory to one with "normal" semantics, we'd only get: [info] Message was: since there is no placeholder for string interpolation. So, in order to make our logging statements portable, we still need to use string interpolations, e.g.:

logger.info("foo was: {}, and bar was: {}",foo,bar)

but that would print out something like

"foo was: {}, and bar was: {}", {foo: "bar"}, {bar: 42}

with the changed semantics for ConsoleLogger. So the question is if we want to accept that quirky "postfix" logging when using ConsoleLogger.

pjazdzewski1990 commented 7 years ago

What I was thinking was this: make it consistent on "write" and convenient on "read". I could be missing something here, but it's tempting to try keep the current format ("Message was: {}") so you can substitute loggers easily while making ConsoleLoggerFactory (or a new logger specific for Scala.js) adjust the output to the browser. @jokade If I find the time, I could try if this is doable and then we could discuss the idea with specific cases in mind. WDYT? Maybe I'm just plain wrong or maybe we can improve slogging a bit :)

jokade commented 7 years ago

@pjazdzewski1990 sure, go ahead! I'm happy about any help to improve this lib. What came to my mind for ConsoleLogger is that we could simply split the first arg at {} and splice the result into the array of arguments.