search

ably / ably-java

Java, Android, Clojure and Scala client library SDK for Ably realtime messaging service
https://ably.com/download
Apache License 2.0
85 stars 39 forks source link

Question: Should the Logger be static #348

Open mattheworiordan opened 7 years ago

mattheworiordan commented 7 years ago

See https://github.com/ably/ably-dotnet/pull/127#issuecomment-327613915

Is it perhaps idiomatic to have a shared static logger between all instances of a library? It seems odd that you could set the log level in one instance of a client and affect the log level in another.

┆Issue is synchronized with this Jira Task by Unito

paddybyers commented 7 years ago

It could be changed to be per-instance but that would mean that lots of classes that currently have log calls (eg Message, ProtocolMessage etc) would either have to have a reference to the AblyRealtime instance, or its Log instance, or many methods on those classes would have to have a Log passed in as an argument. They're both a bit nasty (ie a Message type should be able to be just the members of a message, and associated functionality, and it shouldn't also have to wrap an Ably or Log instance).

As far as being idiomatic goes, it's not really idiomatic to have the log level set by the client library; if we were using a common logging platform such as Log4j (https://github.com/ably/ably-java/issues/64) then the intended way of working would be to configure the log level via that platform's own interface (eg configuration files or environment) instead of through the Ably API. These frameworks are intended to separate the job of configuring the logger from writing code that calls the logger. Eg see http://juliusdavies.ca/logging.html#log4j-program

tcard commented 7 years ago

Alternatively, methods on Message and friends could be moved to the classes acting on them (ie. AblyRest/Realtime and its auxiliary classes) so that they're left just holding the data as POJOs. That's how it is in iOS.

paddybyers commented 7 years ago

Alternatively, methods on Message and friends could be moved to the classes acting on them (ie. AblyRest/Realtime and its auxiliary classes) so that they're left just holding the data as POJOs. That's how it is in iOS.

Yes, true, we could have Channel.decodeMessage() instead of Message.decode(). But I do feel that the transformation that that performs - ie changing the data based on encoding etc - isn't anything to do with a Channel and really is Message functionality.

sacOO7 commented 1 year ago

Can introduce buggy behaviour as per discussion -> https://github.com/ably/ably-java/pull/964#pullrequestreview-1529069701