Skip to content
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.

Sign up for GitHub

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

Expose and allow to enable and disable the debug mode at runtime #60

Merged
merged 2 commits into from
Nov 8, 2023
Merged

Expose and allow to enable and disable the debug mode at runtime #60

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
97f975c
Expose and allow to enable and disable the debug mode at runtime
marandaneto Nov 8, 2023
47ad98e
fix format
marandaneto Nov 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
## Next

- Expose and allow to enable and disable the debug mode at runtime ([#60](https://github.com/PostHog/posthog-android/pull/60))

## 3.0.0-beta.4 - 2023-11-08

- Fix leaked resources identified by StrictMode ([#59](https://github.com/PostHog/posthog-android/pull/59))
Expand Down
3 changes: 3 additions & 0 deletions posthog-android/src/test/java/com/posthog/android/PostHogFake.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,4 +86,7 @@ public class PostHogFake : PostHogInterface {
override fun distinctId(): String {
return ""
}

override fun debug(enable: Boolean) {
}
}
4 changes: 4 additions & 0 deletions posthog/api/posthog.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ public final class com/posthog/PostHog : com/posthog/PostHogInterface {
public fun alias (Ljava/lang/String;)V
public fun capture (Ljava/lang/String;Ljava/lang/String;Ljava/util/Map;Ljava/util/Map;Ljava/util/Map;Ljava/util/Map;)V
public fun close ()V
public fun debug (Z)V
public fun distinctId ()Ljava/lang/String;
public fun flush ()V
public fun getFeatureFlag (Ljava/lang/String;Ljava/lang/Object;)Ljava/lang/Object;
Expand All @@ -26,6 +27,7 @@ public final class com/posthog/PostHog$Companion : com/posthog/PostHogInterface
public fun alias (Ljava/lang/String;)V
public fun capture (Ljava/lang/String;Ljava/lang/String;Ljava/util/Map;Ljava/util/Map;Ljava/util/Map;Ljava/util/Map;)V
public fun close ()V
public fun debug (Z)V
public fun distinctId ()Ljava/lang/String;
public fun flush ()V
public fun getFeatureFlag (Ljava/lang/String;Ljava/lang/Object;)Ljava/lang/Object;
Expand Down Expand Up @@ -143,6 +145,7 @@ public abstract interface class com/posthog/PostHogInterface {
public abstract fun alias (Ljava/lang/String;)V
public abstract fun capture (Ljava/lang/String;Ljava/lang/String;Ljava/util/Map;Ljava/util/Map;Ljava/util/Map;Ljava/util/Map;)V
public abstract fun close ()V
public abstract fun debug (Z)V
public abstract fun distinctId ()Ljava/lang/String;
public abstract fun flush ()V
public abstract fun getFeatureFlag (Ljava/lang/String;Ljava/lang/Object;)Ljava/lang/Object;
Expand All @@ -163,6 +166,7 @@ public abstract interface class com/posthog/PostHogInterface {

public final class com/posthog/PostHogInterface$DefaultImpls {
public static synthetic fun capture$default (Lcom/posthog/PostHogInterface;Ljava/lang/String;Ljava/lang/String;Ljava/util/Map;Ljava/util/Map;Ljava/util/Map;Ljava/util/Map;ILjava/lang/Object;)V
public static synthetic fun debug$default (Lcom/posthog/PostHogInterface;ZILjava/lang/Object;)V
public static synthetic fun getFeatureFlag$default (Lcom/posthog/PostHogInterface;Ljava/lang/String;Ljava/lang/Object;ILjava/lang/Object;)Ljava/lang/Object;
public static synthetic fun getFeatureFlagPayload$default (Lcom/posthog/PostHogInterface;Ljava/lang/String;Ljava/lang/Object;ILjava/lang/Object;)Ljava/lang/Object;
public static synthetic fun group$default (Lcom/posthog/PostHogInterface;Ljava/lang/String;Ljava/lang/String;Ljava/util/Map;ILjava/lang/Object;)V
Expand Down
10 changes: 10 additions & 0 deletions posthog/src/main/java/com/posthog/PostHog.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,13 @@ public class PostHog private constructor(
return distinctId
}

override fun debug(enable: Boolean) {
if (!isEnabled()) {
return
}
Comment on lines +531 to +533
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I think I see... we have the isEnabled check so that a developer gets feedback when they're trying to use the SDK incorrectly?

I guess over time it'd be nice to make that behaviour less repetitive in the code but total tangent and not blocking here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check is that you cannot flip any flag or set anything in case the SDK is disabled because at this point there's no active configuration.

So every method calls isEnabled first thing and early returns if the SDK was not enabled yet.

I don't know what you mean by less repetitive in the code, it's a guard check that has to be within every single public API.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A common C#/Java style for something like this is have an interface, an implementing class, and a wrapping class.

The wrapping class applies the check (or logs the info or whatever the repeated activity is) and then delegates through to the implementing class.

So, the implementing class doesn't have the repetition and the interface protects you from not having the check in the wrapping class.

It's differently noisy though so I'm not saying we should do this now, but if this keeps growing then it might be less noisy to move the isEnabled check to a wrapper

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh now I got what you meant, 100%, yeah at some point this is a good idea, but not all the time since we have to watch out for bundle size, the compiler usually inlines stuff like that (I hope), but more classes/abstractions usually increase much more, will pay attention if I have to do it again.

config?.debug = enable
}

public companion object : PostHogInterface {
private var shared: PostHogInterface = PostHog()
private var defaultSharedInstance = shared
Expand Down Expand Up @@ -662,5 +669,8 @@ public class PostHog private constructor(
}

override fun distinctId(): String = shared.distinctId()
override fun debug(enable: Boolean) {
shared.debug(enable)
}
}
}
5 changes: 5 additions & 0 deletions posthog/src/main/java/com/posthog/PostHogInterface.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,4 +145,9 @@ public interface PostHogInterface {
* Returns the registered [distinctId] property
*/
public fun distinctId(): String

/**
* Enables or disables the debug mode
*/
public fun debug(enable: Boolean = true)
}
24 changes: 24 additions & 0 deletions posthog/src/test/java/com/posthog/PostHogTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -784,4 +784,28 @@ internal class PostHogTest {

sut.close()
}

@Test
fun `enables debug mode`() {
val config = PostHogConfig(apiKey)
val sut = PostHog.with(config)

sut.debug(true)

assertTrue(config.debug)

sut.close()
}

@Test
fun `disables debug mode`() {
val config = PostHogConfig(apiKey).apply {
debug = true
}
val sut = PostHog.with(config)

sut.debug(false)

assertFalse(config.debug)
}
}