holyturt / SwiftOverpassWrapper

A wrapper of the Overpass API for Swift.
MIT License
16 stars 2 forks source link

Make it easier to utilize the XML parsing from within other projects #14

Closed wtimme closed 6 years ago

wtimme commented 6 years ago

Use Case

I'm currently working on a framework to query the OpenStreetMap API v0.6, and would like to utilize the XML parsing that SwiftOverpassWrapper is already doing.

The Issue

I added the framework to my dependencies, but quickly noticed that I am not able to benefit from our unit-tested XML parsing, since the conversion from Data to an AEXMLDocument, followed by enumerating all the objects from the DataResponse is hidden in the hidden initializer of OverpassResponse:

internal init(response: DataResponse<String>, requestQuery: String) { }

Improve OverpassResponse

OverpassResponse doesn't seem like a bad place to do the parsing of an XML response. A few properties, however, are of no use to me, namely

By removing requestQuery, we remove the need to pass it to the initializer:

internal init(response: DataResponse<String>) { }

Let's have a look at how the response argument is being used:

self.xml = String(data: response.data!, encoding: String.Encoding.utf8)!
let xmlDoc = try AEXMLDocument(xml: self.xml)

So what we actually need is the XML String - let's update the initializer to reflect that:

internal init(xml: String) { }

Now all we need to do is make the initializer publicly accessible:

public init(xml: String) { }

Besides making the XML parsing available publicly, this would allow us to finally unit test that last bit of the response: combined lists of nodes, ways and relations. What do you think, @holyturt?

holyturt commented 6 years ago

Throwing Error is what I wanted. I definitely would like to adopt it! These properties, requestQuery and xml, are made for checking actual query and response when the API returns errors but it seems inappropriate for here and is for developers of this project rather than users. I think such properties have to move into Error if they are needed. Therefore they should be removed from OverpassResponse and make the initializer have the response only. What do you think, @wtimme?

holyturt commented 6 years ago

Making the publicly accessible is looks great! We need testability. 😉

wtimme commented 6 years ago

I've responded to your suggestion over at the pull request (#15). Once it is merged we can close this issue. 👍