-
Notifications
You must be signed in to change notification settings - Fork 0
Add near and withinBounds filters with local filtering support #36
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
Generated by 🚫 Danger |
| /// | ||
| /// - Returns: A dictionary representation of the filter in `RawJSON` format. | ||
| public func toRawJSON() -> [String: RawJSON] { | ||
| public func toRawJSONDictionary() -> [String: RawJSON] { |
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 is used for API requests in the Feeds SDK. rawJSON above comes from the FilterValue protocol and is used by toRawJSONDictionary. All in all, toRawJSONDictionary is just a convenience for API requests.
rawJSON was moved to the protocol since then it is better to manage conformance of all the types. Discovered some mapping issues like Int was not correctly encoded to RawJSON thanks to this change (there was a big switch statement before which).
| /// let center = CLLocationCoordinate2D(latitude: 37.7749, longitude: -122.4194) | ||
| /// let region = CircularRegion(center: center, radiusInMeters: 5000) | ||
| /// ``` | ||
| public struct CircularRegion { |
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.
Naming comes from Apple's CLCircularRegion
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.
maybe we move this location stuff in a separate extension? Up to you though.
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.
and how are other SDKs calling this? Better to align on SDKs convention, over Apple's.
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 will have a chat with others about the naming and will rename if needed.
| func contains(_ coordinate: CLLocationCoordinate2D) -> Bool { | ||
| let centerLocation = CLLocation(latitude: center.latitude, longitude: center.longitude) | ||
| let coordinateLocation = CLLocation(latitude: coordinate.latitude, longitude: coordinate.longitude) | ||
| let distance = centerLocation.distance(from: coordinateLocation) |
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 takes the Earth's curvature into account, same as what backend does.
| /// let sw = CLLocationCoordinate2D(latitude: 40.7580, longitude: -73.9855) | ||
| /// let boundingBox = BoundingBox(northeast: ne, southwest: sw) | ||
| /// ``` | ||
| public struct BoundingBox { |
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.
Naming coming from MapBox and other location related frameworks.
martinmitrevski
left a comment
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.
LGTM ✅ Just left 2 small remarks, but feel free to merge.
| /// let center = CLLocationCoordinate2D(latitude: 37.7749, longitude: -122.4194) | ||
| /// let region = CircularRegion(center: center, radiusInMeters: 5000) | ||
| /// ``` | ||
| public struct CircularRegion { |
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.
maybe we move this location stuff in a separate extension? Up to you though.
| /// let center = CLLocationCoordinate2D(latitude: 37.7749, longitude: -122.4194) | ||
| /// let region = CircularRegion(center: center, radiusInMeters: 5000) | ||
| /// ``` | ||
| public struct CircularRegion { |
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.
and how are other SDKs calling this? Better to align on SDKs convention, over Apple's.
|



🔗 Issue Links
Fixes: IOS-1237
🎯 Goal
Add near and withinBounds filters with local filtering support new in feeds backend
🛠 Implementation
Feeds backend added new filter operators
nearandwithin_bounds. One for matching circular regions using Haversine formula (takes Earth's curvature into account), other just matching to bounds.☑️ Contributor Checklist
docs-contentrepo