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.

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

Conversation

marandaneto
Copy link
Member

💡 Motivation and Context

Unblock migration https://github.com/djamoapp/posthog-flutter/blob/ae8b77bb776e4d32882b2f61cdebf9fe45dbae34/lib/src/posthog.dart#L81-L84

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

Copy link
Member

@pauldambra pauldambra left a comment

Choose a reason for hiding this comment

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

👍

Comment on lines +531 to +533
if (!isEnabled()) {
return
}
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.

@marandaneto marandaneto merged commit 8a905bc into main Nov 8, 2023
5 checks passed
@marandaneto marandaneto deleted the chore/expose-debug branch November 8, 2023 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants