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

Expose Placemark.properties #163

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

frederoni
Copy link
Contributor

@frederoni frederoni commented Sep 19, 2018

Fixes #159

I've split up Placemark and GeocodedPlacemark into separate files to improve readability.

The actual changes are:

- superiorPlacemarks = try container.decodeIfPresent([GeocodedPlacemark].self, forKey: .superiorPlacemarks)
+ superiorPlacemarks = try container.decodeIfPresent([Placemark].self, forKey: .superiorPlacemarks)

along with exposing Placemark.properties, which I don't think should be exposed after a second thought because we have aliases to most of these properties. 🤔

cc @1ec5 @riastrad

Also refactored GeocodedPlacemark out of Placemark to improve
readability.
@@ -152,7 +152,7 @@ open class Placemark: NSObject, Codable {
/**
A subset of the `properties` object on a GeoJSON feature suited for Geocoding results.
*/
fileprivate var properties: Properties?
public var properties: Properties?
Copy link
Contributor

Choose a reason for hiding this comment

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

along with exposing Placemark.properties, which I don't think should be exposed after a second thought because we have aliases to most of these properties. 🤔

Yeah, that was my original intention with this architecture. The Geocoding API has always been quite strictly oriented around GeoJSON, but I’ve always felt that clients of the library need a stronger, less general-purpose type system. Which properties don’t have dedicated properties?

@1ec5 1ec5 changed the base branch from master to main October 7, 2020 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants