pep8 compliant and removed bbox

ikks commented 9 years ago

We would need to parse the input to make sure we can apply the bbox, if we intend not to use this for complex queries, it is easier to point people to the overpass API to let them explicitly put the bbox in the general queries.

mvexel commented 9 years ago

I think this needs some discussion. I like the convenience of being able to pass in a bbox without having to construct it as part of the query. And isn't this what this wrapper is all about - convenience? What do you think @itiboi @johnjohndoe @mharnold?

tbolender commented 9 years ago

It is possible to define a different bbox for every query part?

mharnold commented 9 years ago

I'll have some time for this project this afternoon, and will take a look at this.

In the meantime I have been wanting to suggest:

make the raw full unmodified query to overpass part of the exported api
for the higher level get, add options to explicitly control the output format (tacked on to the front , and extended to allow geojson) and the 'print' statement parameters (tacked on to the end of the query.)

I'll write this up more carefully when I have time this afternoon.

Cheers, Michael spiderOSM.org

On Wed, Nov 19, 2014 at 9:07 AM, Martijn van Exel notifications@github.com wrote:

I think this needs some discussion. I like the convenience of being able to pass in a bbox without having to construct it as part of the query. And isn't this what this wrapper is all about - convenience? What do you think @itiboi https://github.com/itiboi @johnjohndoe https://github.com/johnjohndoe @mharnold https://github.com/mharnold?

— Reply to this email directly or view it on GitHub https://github.com/mvexel/overpass-api-python-wrapper/pull/19#issuecomment-63675230 .

tbolender commented 9 years ago

I almost forgot about it: Since we decided to follow PEP-8, I would also propose to rename the class methods to match the lower case style.

mharnold commented 9 years ago

I agree with Igor that implementing bbox in the API is conceptually and practically complicated and not in keeping with the thin wrapper concept. I think addressing bboxes for convenience is best done in queries.py (note MapQuery() already takes bbox args.)

(A side note: It would be cool to support chaining of the queries e.g. A MapQuery followed by a WayQuery() to restrict the Way search to the MapQuery bbox. I haven't taken a close look at this, but I think this is how the overpass API is setup to work.)

The way I conceptualize the API.Get() (should be API.get() in pep8 style I think): prefix: "out format;" - generated for the user. base query: user supplied or generated in query.py suffix: "out options;" - generated for the user.

In addition we do additional format translation work: for json (convert from string to internalized dict format) for geojson (a bit more conversion work.)

To support this I suggest API.get() take keyword args documented as follows:

""" RESPONSE FORMAT (generates 'out format;' query prefix.)

format = 'xml': result is xml (python string) 'json': result is json (python dict) 'geojson': result is geojson (python dict, NOTE: generated by overpass wrapper for convenience - not directly part of overpass api)

PRINT OPTIONS (generates 'out options;' query suffix.)

verbosity= 'ids': Print only the ids of the elements.

'skel': Print also the information necessary for geometry. These are also coordinates for nodes and way and relation member ids for ways and relations.

'body': Print all information necessary to use the data. These are also tags for all elements and the roles for relation members.

'tags': Print only ids and tags for each element and not coordinates or members.

'meta': Print everything known about the elements. This includes additionally to body for all elements the version, changeset id, timestamp and the user data of the user that last touched the object.

derive= 'bb': Adds the bounding box of each element to the element. For nodes this is equivalent to "geom". For ways it is the enclosing bounding box of all nodes. For relations it is the enclosing bounding box of all node and way members, relations as members have no effect.

'center': This adds the center of the above mentioned bounding box to ways and relations. Note: The center point is not guaranteed to lie inside the polygon (example).

'geom': Add the full geometry to each object. This adds coordinates to each node, to each node member of a way or relation, and it adds a sequence of "nd" members with coordinates to all relations.

sort= 'asc': Sort by object id. 'qt': Sort by quadtile index; this is roughly geographical and significantly faster than order by ids. """

In addition I suggest that API._GetFromOverpass() be renamed API.raw_get() That is that it be exported as part of the API. This is convenient for those who prefer to work directly with the overpass api and don't want to worry about how/what is done in API.Get()

Cheers, Michael spiderOSM.org

On Wed, Nov 19, 2014 at 12:02 PM, Tim Bolender notifications@github.com wrote:

I almost forgot about it: Since we decided to follow PEP-8, I would also propose to rename the class methods to match the lower case style.

— Reply to this email directly or view it on GitHub https://github.com/mvexel/overpass-api-python-wrapper/pull/19#issuecomment-63703616 .

mvexel commented 9 years ago

Excellent suggestions @mharnold :+1:

Agreed that we should get rid of Capitalized function names.
Agreed we should export a raw query method - suggestions for a name?
Agreed re: the response format and the print options.
Also yes we should get rid of the bounding box, it is conceptually too hard to come up with something that will consistently work. My thought was that you could define a bbox globally for your query but that does not seem to be possible.

mvexel commented 9 years ago

@ikks would you have time to rebase this? If not I will. Apologies.

mharnold commented 9 years ago

RE: RAW QUERY METHOD NAME raw_get() - succinct and I think clear? low_level_get() - less succinct but maybe clearer?

RE: THE RESPONSE FORMAT AND PRINT OPTIONS I can implement these if you like. :)

RE: API BBOX I certainly see the appeal. Too bad it doesn't seem to meld in well. If things were setup to allow instances of the classes in queries.py to be concatenated (I think this is consistent with how the overpass API is meant to work?) MapQuery() might be renamed BBoxQuery() and the desired functionality be provided in that way? But I haven't really thought this through!

Cheers, Michael spiderOSM.org

On Mon, Nov 24, 2014 at 3:05 PM, Martijn van Exel notifications@github.com wrote:

Excellent suggestions @mharnold https://github.com/mharnold [image: :+1:]

Agreed that we should get rid of Capitalized function names.

Agreed we should export a raw query method - suggestions for a name?

Agreed re: the response format and the print options.

Also yes we should get rid of the bounding box, it is conceptually too hard to come up with something that will consistently work. My thought was that you could define a bbox globally for your query but that does not seem to be possible.

— Reply to this email directly or view it on GitHub https://github.com/mvexel/overpass-api-python-wrapper/pull/19#issuecomment-64281795 .

mvexel commented 9 years ago

I merged this outside of the PR adding a few minor P3 compatibility issues but it did not get picked up here.

@mharnold I like raw_get() and will change method names in a PR today.

mvexel / overpass-api-python-wrapper

pep8 compliant and removed bbox #19