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

Feature should conform to Identifiable #165

Open
1ec5 opened this issue Oct 6, 2021 · 1 comment
Open

Feature should conform to Identifiable #165

1ec5 opened this issue Oct 6, 2021 · 1 comment
Labels
jira-sync-complete op-ex Refactoring, Tech Debt or any other operational excellence work. question

Comments

@1ec5
Copy link
Contributor

1ec5 commented Oct 6, 2021

The Swift standard library defines an Identifiable protocol that provides for an id property. The documentation for this property says the ID’s value can be as unique or as commonplace as the containing type defines it to be. That would make it well-suited for the id property on Feature objects in GeoJSON.

Turf currently stores this property in Feature.identifier. Perhaps we should rename identifier to id and have Feature conform to Identifiable. This would make the identifier more discoverable. However, GeoJSON feature IDs are optional, so the ID associated type would need to be Optional<FeatureIdentifier>, which is probably not what most Identifiable consumers would expect.

The name identifier follows the Cocoa naming convention that abbreviations such as “ID” should always be spelled out. To my knowledge, the Cocoa convention was motivated by the need to avoid collisions with the Objective-C keyword id and the awkwardness of a property named ID, capitalized according to another Cocoa convention. These considerations are less relevant to Turf, which has never supported Objective-C. The Swift naming guidelines make an exception for “embracing precedent”, and there’s plenty of precedent for “ID”.

To maintain backwards compatibility, we could retain the identifier property as a deprecated computed property based on id. It’s unclear if the Identifiable conformance would be a backwards-incompatible change, but we could probably get away with it because Identifiable is still seldom used at this point.

/cc @mapbox/navigation-ios @mapbox/maps-ios

@1ec5 1ec5 added question op-ex Refactoring, Tech Debt or any other operational excellence work. labels Oct 6, 2021
@1ec5
Copy link
Contributor Author

1ec5 commented Oct 15, 2021

Per @bamx23, Identifiable is used by SwiftUI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jira-sync-complete op-ex Refactoring, Tech Debt or any other operational excellence work. question
Projects
None yet
Development

No branches or pull requests

2 participants