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

Ported the turf-boolean-point-in-polygon function to Turf Swift. #64

Merged
merged 2 commits into from
Sep 18, 2018

Conversation

SandyChapman
Copy link
Contributor

Copy link
Contributor

@frederoni frederoni left a comment

Choose a reason for hiding this comment

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

Thanks a lot for porting this feature.
Almost there, just a few nits.

import CoreLocation
#endif

public class BoundingBox {
Copy link
Contributor

@frederoni frederoni Sep 15, 2018

Choose a reason for hiding this comment

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

All other geometries and GeoJSON features are structs, let's stick with that since we don't provide support for Obj-C for now. This bounding box should also conform to Codable.


// MARK: - Private

let minLat: Double
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we consolidate these 4 min/max properties into a northWest southEast CLLocationCoordinate2D for concistency with many of Mapbox’s libraries?


public class BoundingBox {

public init?(points: [CLLocationCoordinate2D]?) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: public init?(from coordinates: [CLLocationCoordinate2D]?)

perhaps an additional initializer:
public init(_ northWest: CLLocationCoordinate2D, _ southEast: CLLocationCoordinate2D)?

*
* Ported from: https://github.com/Turfjs/turf/blob/e53677b0931da9e38bb947da448ee7404adc369d/packages/turf-boolean-point-in-polygon/index.ts#L31-L75
*/
public func contains(point: CLLocationCoordinate2D, ignoreBoundary: Bool = false) -> Bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: contains(_ coordinate: CLLocationCoordinate2D

* Ported from: https://github.com/Turfjs/turf/blob/e53677b0931da9e38bb947da448ee7404adc369d/packages/turf-boolean-point-in-polygon/index.ts#L31-L75
*/
public func contains(point: CLLocationCoordinate2D, ignoreBoundary: Bool = false) -> Bool {
let bbox = BoundingBox(points: self.coordinates.first)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: self is inferred and can be dropped. (×2)

*
* Ported from: https://github.com/Turfjs/turf/blob/e53677b0931da9e38bb947da448ee7404adc369d/packages/turf-boolean-point-in-polygon/index.ts#L77-L108
*/
public func contains(point: CLLocationCoordinate2D, ignoreBoundary: Bool = false) -> Bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: contains(_ coordinate: CLLocationCoordinate2D

}
}

public func contains(point: CLLocationCoordinate2D) -> Bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: contains(_ coordinate:

@frederoni
Copy link
Contributor

This PR partially fixes #50 and #33

Renamed several methods to use identifier `coordinate` instead of `point`.
Made `BoundingBox` a `Codable` `struct`.
Removed several instances of an explict usage of `self`.
@SandyChapman
Copy link
Contributor Author

SandyChapman commented Sep 17, 2018

@frederoni : I believe all your feedback has been addressed. Can you please re-review and let me know if there are additional changes requested?

@frederoni frederoni merged commit 1ee3259 into mapbox:master Sep 18, 2018
@frederoni frederoni mentioned this pull request Sep 18, 2018
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