segmentio / Analytics-CSharp

The hassle-free way to add Segment analytics to your C# written app (.Net/Xamarin/Unity).
MIT License
21 stars 8 forks source link

Make public members of Analytics class virtual #73

Closed jn011 closed 1 year ago

jn011 commented 1 year ago

Hi there!

I've recently started migrating to this library for integration and had noticed that it does not expose an interface which can be easily used to interact with the class.

I have created one called IAnalytics in this PR. This could potentially be used for dependency injection and also has the benefit of making it more testable.

Please let me know if you have any questions or thoughts.

Thanks!

wenxi-zeng commented 1 year ago

hey @jn011, thanks for raising this pr. we purposely made Analytics a concrete class, because it's not expected to have different behaviors in implementation. for unit test, one can always inject a concrete Analytics instance, and because Analytics runs asynchronously, it should not make any difference in app's logic.

however, you can always set Analytics.Enable = false if you worry about the overhead in tests. you can also pass useSynchronizeDispatcher = true in config, if you wanna make Analytics to run synchronous.

let us know if you feel the above does not solve your use case. we will reopen the pr.

jn011 commented 1 year ago

Hi @wenxi-zeng

Thanks for the background info, that makes sense.

Although it doesn't solve my use case since some concrete methods like track and identify are not marked as virtual which means they can't be easily unit tested.

My next thought Is could we make these Methods virtual?

This would be similar to what we have for Flush at the moment.

wenxi-zeng commented 1 year ago

@jn011 yeah. making the methods all virtual is a good suggestion. would you mind to update the pr with that change? happy to marge that in! thanks!

jn011 commented 1 year ago

@wenxi-zeng sounds good. I've just updated my branch :)

wenxi-zeng commented 1 year ago

@jn011 thanks for your contribution! this change will be released along with the feature request of adding support for sync flush