Feature: Add a LoggingInterceptor #80
Conversation
…terface and can be used to monitor the method calls on a bound view.
ghost
added
the
ghost
requested a review
from 
|
|
||
| private final boolean mEnableLogging; | ||
|
|
||
| public LoggingInterceptor(final boolean enableLogging) { |
There was a problem hiding this comment.
add constructor documentation with code sample.
// MyActivity extends TiActivity
public MyActivity() {
addBindViewInterceptor(new LoggingInterceptor(BuildConfig.DEBUG));
}| return wrappedView; | ||
| } | ||
|
|
||
| private static String parseParams(Object[] a, int maxLenOfParam) { |
|
Should we put it in a separate module like okhttp-logging-interceptor? Does anyone has a option on this? |
|
Yes, I've already thought about it. |
Add logger in constructor
|
I added an updated version with improved logging I found in a stale branch. And added some tests :) |
| import static org.mockito.Mockito.never; | ||
| import static org.mockito.Mockito.verify; | ||
|
|
||
| public class LoggingInterceptorTest { |
| } | ||
|
|
||
| @Test | ||
| public void cropLongParams() throws Exception { |
There was a problem hiding this comment.
Haven't we agree that all tests should start with testXXX? 🤔
| return wrappedView; | ||
| } | ||
|
|
||
| private static String parseParams(Object[] methodParams, int maxLenOfParam) { |
There was a problem hiding this comment.
Should be part of the MethodLoggingInvocationHandler.
| } | ||
| } | ||
|
|
||
| private static final String TAG = MethodLoggingInvocationHandler.class.getSimpleName(); |
There was a problem hiding this comment.
The TAG is wrong here.
Has to be LoggingInterceptor.class.getSimpleName()
| throws Throwable { | ||
|
|
||
| try { | ||
| mLogger.log(Log.VERBOSE, TAG, toString(method, args)); |
There was a problem hiding this comment.
This TAG is part of the LoggingInterceptor class...Is that expected? 🤔
There was a problem hiding this comment.
yes, the InvocationHandler is just a implementation detail. You add a LoggingInterceptor and you expect this interceptor to log something.
|
|
||
| StringBuilder b = new StringBuilder(); | ||
| for (int i = 0; ; i++) { | ||
| final Object arg = methodParams[i]; |
| for (int i = 0; ; i++) { | ||
| final Object arg = methodParams[i]; | ||
|
|
||
| final String param; |
There was a problem hiding this comment.
this is more a paramAsString
| int iMax = methodParams.length - 1; | ||
|
|
||
| StringBuilder b = new StringBuilder(); | ||
| for (int i = 0; ; i++) { |
There was a problem hiding this comment.
Do we need that infinite loop really?
Can't we use the condition i != iMax here?
We return then b.toString() after the loop and remove the last three characters?!
It sounds for me for a cleaner solution
| @@ -99,6 +125,10 @@ public static void i(final String tag, final String msg) { | |||
| * </code> | |||
| */ | |||
| public static void setLogger(@Nullable final Logger logger) { | |||
| if (logger == TI_LOG) { | |||
| throw new IllegalArgumentException( | |||
| "Recursion warning: You can't use TI_LOG as Logger for TiLog"); | |||
There was a problem hiding this comment.
If we can't do it. Why do we have it then? 🤔
Can be removed? 🌮
There was a problem hiding this comment.
It's a implementation of this interface which can be used everywhere except for this location which would result in recursion.
I really like that the LoggingInterceptor logs to TiLog by default using the TiLog.TI_LOG.
| public static Logger TI_LOG = new Logger() { | ||
| @Override | ||
| public void log(final int level, final String tag, final String msg) { | ||
| TiLog.log(level, tag, msg); |
There was a problem hiding this comment.
I recomment to remove the "self login".
Instead i would add a System.out.print(level, tag, msg) (or smth)
Adds a
LoggingInterceptorwhich implements theBindViewInterceptor-Interface and can be used to monitor the method calls on a bound view.This Class was originally implemented by @passsy and I made only a few changes:
instead of accessing the build config to determine if the logging should be enabled this is now controlled via the flag
mEnableLoggingwhich is set via the classes constructor.