-
Notifications
You must be signed in to change notification settings - Fork 50
refactor(multi): simplify API with functional options pattern #444
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #444 +/- ##
==========================================
+ Coverage 82.89% 83.07% +0.17%
==========================================
Files 27 27
Lines 2327 1985 -342
==========================================
- Hits 1929 1649 -280
+ Misses 363 299 -64
- Partials 35 37 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/gemini review |
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.
Code Review
This pull request implements the Tracker interface for the multi-provider, enabling the forwarding of tracking calls to all ready providers that support tracking. The changes include adding the Tracker interface implementation to the Provider struct, modifying the NewProvider function to handle provider hooks correctly, updating the event handling logic in the Init function, and implementing the Track function to forward tracking calls. Additionally, the pull request includes corresponding unit tests to verify the functionality of the Tracker implementation.
0769cea to
ac0a95e
Compare
|
/gemini review |
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.
Code Review
This pull request introduces a functional options pattern to simplify the API of the multi-provider, replaces the ProviderMap type with variadic functional options, changes NamedProvider from a struct to an interface, updates the NewProvider signature, adds Track method support, and updates tests and documentation. The changes enhance the flexibility and maintainability of the multi-provider. I have provided comments to address potential issues related to error handling, interface usage, and code clarity.
10d7a98 to
7a2c8ce
Compare
|
I think @thisthat was just mentioning the other day we should do this. |
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.
I have no issues with the changes but I think the title should be feat(multi): add tracking, functional options or something like that, since we added that new functionality.
Also it's probably worth noting there's breaking changes here with the multi-provider, but I think that's fine since we explicitly said it's experimental, and it's an auxiliary feature (so let's not add a ! to the title).
08f4ade to
7fd7ab6
Compare
This PR