Wrap Search Configs #238
Wrap Search Configs #238
Conversation
src/MapzenResponse.swift
Outdated
| import Foundation | ||
| import Pelias | ||
|
|
||
| public class MapzenResponse : NSObject { |
There was a problem hiding this comment.
This should probably be something more descriptive like SearchResponse.
Also it's not practice in swift to prefix names with things like MZ or Mapzen because namespaces are implicit. So if there's a namespace conflict you would use ModuleName.Class.Function() instead of Class.Function(). So in general I would remove that prefix from these classes / structs / etc. This is something I failed to understand while building Pelias, so lets fix that here :)
There are always exceptions however. For example the MapzenManager is a good one since it's a state manager for Mapzen services, but otherwise I think we can be a little more loose with naming.
There was a problem hiding this comment.
While writing my swift code I had objective-c's lack of namespaces in mind (eek!). I think you're right, this can be more descriptive, I'll change it to SearchResponse. I also agree re exceptions like MapzenManager.
...combining parts of what we are both thinking, how do you feel about adding an MZ prefix for the objective-c class names? So something like this:
@objc(MZSearchResponse)
public class SearchResponse: NSObject
src/MapzenPlaceConfig.swift
Outdated
| import Foundation | ||
| import Pelias | ||
|
|
||
| public class MapzenPlaceConfig : NSObject { |
There was a problem hiding this comment.
What are you thoughts on moving all of these into a single swift file? It's common practice to have common structs / class grouped in the same file in Swift.
There was a problem hiding this comment.
Its against what inherently feels natural to me (its overwhelming to see large files) but if its swift common practice I'll move it into one file.
src/MapzenSearchDataConverter.swift
Outdated
| @@ -0,0 +1,153 @@ | |||
| // | |||
| // MapzenSearchDataConverter.swift | |||
| // Pods | |||
There was a problem hiding this comment.
Nit: Just noticed the Pods name here. This should be the project name, so Mapzen-ios-sdk or something similar.
There was a problem hiding this comment.
Will update to ios-sdk
src/MapzenSearchDataObjects.swift
Outdated
| import Foundation | ||
| import Pelias | ||
|
|
||
| public class MzSearchRect: NSObject { |
There was a problem hiding this comment.
Yeah like I said above we can probably remove all of the prefixes here and then for things like SearchBoundaryCircle which will conflict on the namespace you should be able to reference it by module name like Pelias.SearchBoundaryCircle
There was a problem hiding this comment.
How do you feel about changing the sdk module name from ios_sdk to Mapzen?
| let center = MzGeoPoint(latitude: 70.0, longitude: 40.0) | ||
| let searchCircle = MzSearchCircle(center: center, radius: 8) | ||
| let circle = SearchBoundaryCircle(center: MapzenSearchDataConverter.unwrapPoint(center), radius: 8) | ||
| XCTAssertEqual(searchCircle.circle, circle) |
There was a problem hiding this comment.
You should probably use XCTAssertEqualWithAccuracy() here and for the accuracy part use DBL_EPSILON to avoid possible compiler weirdness with floats, since checking equality with floats tends to be hit-or-miss.
There was a problem hiding this comment.
I thought that weirdness was just when the float was returned from TGMapViewController's getPosition?
There was a problem hiding this comment.
Indeed, but in general you shouldn't do direct equality checks on floats without an accuracy check in place (which is why I started down that path in the first place).
| XCTAssertEqual(wrapped.dataSource, .openStreetMap) | ||
| XCTAssertEqual(wrapped.layer, .venue) | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm not seeing whitespace on this line:
Was it on a different line?
Overview
This adds objective-c support for part of the Pelias API. A follow-up PR will integrate these changes into a new
MapzenSearchobject and complete objective-c compatibility.Proposed Changes
Peliasstructs inNSObjectequivalentsMapzenSearchDataConverterFuture work:
Closes #80