-
Notifications
You must be signed in to change notification settings - Fork 120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce setDebugLoggingEnabled API #549
Conversation
@@ -219,7 +228,7 @@ private void sendEventsWrapped(Vector<Hashtable<String, Object>> events, Callbac | |||
String url = getEventsEndpoint() + "/events/v2?access_token=" + getAccessToken(); | |||
|
|||
// Extra debug in staging mode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is now lying. This is an example of why I'm not a big fan of comments
@@ -219,7 +228,7 @@ private void sendEventsWrapped(Vector<Hashtable<String, Object>> events, Callbac | |||
String url = getEventsEndpoint() + "/events/v2?access_token=" + getAccessToken(); | |||
|
|||
// Extra debug in staging mode | |||
if (isStagingEnvironment()) { | |||
if (isDebugLoggingEnabled()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zugaldia
If we're going to go this way, we need to change
Line 255 in e4842cb
client.setStagingEnvironment(true); |
with MapboxTelemetry#setDebugLoggingEnabled(true)
and deprecate/adapt
Line 38 in e4842cb
private boolean stagingEnvironment = false; |
its getter and its setter
Lines 76 to 82 in e4842cb
public boolean isStagingEnvironment() { | |
return stagingEnvironment; | |
} | |
public void setStagingEnvironment(boolean stagingEnvironment) { | |
this.stagingEnvironment = stagingEnvironment; | |
} |
Another possibility is adding || isStagingEnvironment()
and not break anything 😉
Right now, we're not logging when in staging mode if we don't set debugLoggingEnabled
to true
which is not desired because we're not preserving previous functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, let's do || isStagingEnvironment()
for now. Fixing up in a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The best comment is a good name for a method or class.
or variable in this case 😛
👀 Code Smells 👉 Comments
This API lets us manually control the verbosity of HTTP requests.