-
Notifications
You must be signed in to change notification settings - Fork 44
feat!!: add v30 changes #107
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
base: master
Are you sure you want to change the base?
Conversation
3f69914 to
733dab4
Compare
| /** | ||
| * AnalyticsEvents constructor. | ||
| * | ||
| * @param ApiCall $apiCall |
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.
Any reason these definitions are removed?
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.
added them back 26e6f80. The entire file was re-written from scratch, but the diff algorithm shows it as modified
src/AnalyticsRules.php
Outdated
| public function offsetSet($offset, $value): void | ||
| { | ||
| $this->analyticsRules[$offset] = $value; | ||
| // Not implemented for read-only access |
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.
Why not? It would be nice for users to be able to access it via array notation for read-only access right?
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 goes back to a really old issue on Laravel Scout, where we only checked for memory instances of collections and as a result, queue workers tried to access collections that didn't exist. Added it back to keep backwards compatibility though
|
Maybe you can consider making an RC tag of this so its a bit smoother to test it together with typesense v30? |
Change Summary
Add the v30 changes to the client:
PR Checklist