holyturt / swiftoverpasswrapper Goto Github PK

Since I'm using not only Overpass but also the OpenStreetMap API in my own app, I find the current elements naming scheme a bit misleading. What do you think about dropping the Overpass prefix for the classes, @holyturt? For example, OverpassNode would be called just Node.

Looking forward to your feedback!

From issue #3:

I don't know this topic is important but what do you think about using Overpass XML for the API. When I started this project, I picked it without thinking so much. However, if using Overpass QL is more convenient in future, I could switch to it. Because the project seems still simple enough to do it.

I haven't had a closer look on the query language vs. the XML, but if we go with the rules it may save us a conversion step if we also decide to switch to Overpass QL.

Today I met the author of StreetComplete, @westnordost. I told him about SwiftOverpassWrapper and my efforts to create a basic client for OpenStreetMap's REST API v0.6. He suggested to consider adding the functionality to the SwiftOverpassWrapper repository.

What do you think, @holyturt?

From issue #3:

Maybe using a database such as SQLite. Could be useful for offline applications. If we didn't want to include a cache in this project, we could still make sure that our objects are easy to cache and that a cache can optionally be added to the data management object.

Looking ahead, this project going to support caching map data.

Caching data seems not necessary for every apps. But it is an important feature for many apps which this project relate to. So, in my opinion, it should be optional.

Yes, I agree. That's why it would be nice to work with protocols, as it allows other developers to optionally add their own cache. I haven't worked on this yet, though, because I feel like the first point (having a dedicated object for relations lookup) needs to be addressed before we can move on to caching.

And it will be optional for now.

From issue #3:

I've already started adding tests for the XML parsers, but noticed that in order to create the node, entity or way, I needed to always provide the OverpassResponse that the XML came with. This doesn't feel quite right. Have a look at this snippet from one of the test cases. xmlElement is a AEXMLElement that I read from a local file.

So just for testing the XML parsing, I need to create an OverpassResponse that I don't actually need for the XML parsing. But when wanting to lookup releations, the models need to have a reference to the response. So what could be a solution?

Instead of having the reference to the response that an item was retrieved with, I suggest to use a dedicated object that can be queried for this information. Internally, it would keep track of the "world" that it knows about, and could lazily lookup data when needed.
This serves three purposes:

• Keeping the models as stupid possible, having only actual data attached to them. This makes unit testing easier and reduces the complexity of the models. Additionally, it improves readability, which is always good to have, as there's no logic in those objects.
• Letting the app "learn" about the world: With each query, new relations might be discovered, and "old" references to response instances don't contain the whole picture.
• Preparing/allowing for the addition of optional caching: This is something that I'm currently implementing in my own app, but would be nice to have shared with everyone.

So do you have any ideas you can share for implementing the dedicated object from your works or thoughts?

Yes, I do. First of all, I'd create a central place for storing the results of all the responses so far, and already started working on a protocol. Take a look:

public protocol OverpassEntityManaging {
    /// Provides all available entities from storage.
    var entities: [OverpassEntity] { get }
    
    /// Adds a list of entities to the storage.
    /// The method will filter and list and only add those nodes that are either completely new or updated.
    ///
    /// - Parameter entities: The entities to process.
    func add(_ entities: [OverpassEntity])
}

As you can see, the object takes elements of type OverpassEntity and processes them. In my current implementation, add(_:) already handles multiple instances of the same entity being added, and maintains a list of only the newer ones (= those with a greater version).
I plan on having yet another object that contains a OverpassEntityManaging object for each of the elements (way, node and relation), but haven't started implementing it yet. That object can then have methods for looking up related nodes, ways and relations. I'll come back to you once I have some results to show.

Steps to reproduce:

// 1. Create an `OverpassQuery`
let query: OverpassQuery = ...

// 2. Create an `OverpassAPI`
let api: OverpassApi = ...

// 3. Fetch data using `OutputVerbosity.meta` 
api.fetch(query, verbosity: .meta) { response in
    // Handle the response
}

response.nodes contains a list of OverpassNode objects.

Observed behaviour

Unfortunately, when parsing the response XML, meta information (e. g. version) is thrown away and cannot be accessed using the OverpassNode.

Expected behaviour

OverpassNode should contain the meta information from the response.

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

• requestQuery and
• xml

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?

I would love to have some CI that runs the tests when a new pull request is created, displaying the test results right here on GitHub so that we can easily tell when something's breaking.

Do you know of any (possibly free 😅) CI services that we could set up, @holyturt?
If I find the time I'll do some research myself and share the results.

CI Wishlist

For each pull request (on creation and/or on update),

• run the unit tests
• profile costly operations (measureBlock)
• collect warnings

With all the recent changes, we should release a new version.

Version Number

We've updated the models and the queries, which are breaking API changes for clients. If we were to follow Semantic Versioning 2.0.0, the new release would be 2.0.0. What do you think, @holyturt?

Tasks

• Update spec.version in SwiftOverpass.podspec and commit
• Tag that commit

From issue #3:

We have a plan to make "rules" to help to make it more readable/editable and unit tests easier like StreetComplete do.

This is something that StreetComplete has and what is really nice for reading/editing queries. Have for example a look at the tag filter for the AddBabyChangingTable quest:

(type:node or type:way)
and
diaper!=*
and
(
    amenity = toilets
    or
    (
        (
            amenity~="restaurant|cafe|fuel|fast_food"
            or
            shop~="mall|department_store"
        )
        and
        name=*
        and
        toilets=yes
    )
)

As an added bonus, the StreetComplete has a test case for their parser, allowing for TDD. 🤓
A proper parser allows for more complex rules than we can currently do in code. Furthermore, the queries could be built using an external tool (such as overpass turbo) and can be exchanged.

This is something for the future, and could even reside in its own little project, as SwiftOverpassWrapper doesn't necessarily depend on it. Before adding this new feature, I'd make sure that the rest of the code base is in a way that can be easily extended.

On XCode 13.2.1 you will get:

As the developer using SwiftOverpassWrapper, I want to be able to just depend on the models, so that I don't include the whole Overpass communication in my project.

I'm currently working on a Swift framework that helps with the OSM REST API v0.6. In my framework, I would like to also make full use of the models within SwiftOverpassWrapper.

Tasks

• Make the designated initializers of the models public
• Group all models in a dedicated directory
  • OverpassElement
  • OverpassNode
  • OverpassWay
  • OverpassRelation
• Move OverpassResponseElementsProviding to that folder as well (Node, Way and Relation depend on it. *)
• Add a Subspec to the Podspec file
• Update the README.md to include instructions on how to use the Subspec

* This is a candidate for a refactoring. It just feels like there must be a smarter way to do it that allows us to decouple the elements from the response a bit more.

As a developer, I want my code to be automatically formatted so that the project has a consistent style.

Acceptance criterias

• When building the project, SwiftLint should autocorrect the whole project
• The Pods/ directory should not be autocorrected
• The whole project is autoformatted once as part of this story (for a "fresh" start)

Open questions

• Should the binary (> 10 MB) be checked into the repository?
• Are there any rules that we want to opt-in to?

When user makes relation between queries, it may be like:

let nodeQuery = SwiftOverpass.query(type: .node)
let wayQuery = nodeQuery.related(.way)

It will work correctly, but the method related(_:) may cause an error inside the API server if user codes like:

let nodeQuery = SwiftOverpass.query(type: .node)
let backwardsQuery = nodeQuery.related(.backwards) // invalid relationship(node-backwords)

As above, user can make wrong relationships(e.g. node-backwards, node-node etc.) easily. First, I made it can throw an error when it's going wrong way. But it seems better to be restructured.

I think the cause is sharing related(_:) in the all query classes which implement OverpassQuery. Therefore, I split up the method into dedicated methods(e.g. node(:_), way(:_), backwards(:_) etc.) and set them into proper classes. The details are in the PR #8.

This will be breaking changes for making query instances.

// before
let node = SwiftOverpass.query(type: .node)
let way = node.related(.way)

// after
let node = NodeQuery()
let way = node.way()

Therefore, I'd like to have opinions/thoughts.

Is the models used complete for any json object that overpass returns? If so is there a offical json that contains all possible return values somewhere - or can I used the swiftModels as reference in my C# project and assume it's complete?

Hey @holyturt,

Thanks for your effort, and thanks for accepting the pull requests!

My usecase for the framework is querying the Overpass server for nodes in a bounding box, basically for performing the bare requests. I store nodes in memory and keep track of them in memory, merging new ones with existing ones, avoiding duplicates, etc by myself. Therefore, I'm not dependend on the methods for loading related entities, such as loadRelatedWays().

How do you use this framework? I'm asking because I'd like to exchange ideas and opinions. Do you have any experience that you can share? Thanks in advance for your response!

As a user, I want an app which downloads data from Overpass to indicate that data is currently being downloaded, so that the app feels more responsive to me.

In my own app, I've implemented a simple counter that is increased before a request is started, and decreased when the request finished (in the completion closure).
It would be great if such functionality was offered by OverpassAPI, e. g. in the form of a publicly accessible

var areRequestsInProgress: Bool {
    return 0 < numberOfRequestsInProgress
}

What do you think, @holyturt? I have no idea whether my approach is the way to go. The last thing that I want is an infinite loading spinner. Is the completion closure of the network request always executed? Your input is highly appreciated!