mvexel / overpass-api-python-wrapper

Python bindings for the OpenStreetMap Overpass API
Apache License 2.0
369 stars 90 forks source link

Merge Wille's fork and improvements #36

Closed mvexel closed 8 years ago

mvexel commented 8 years ago

I have made an effort to cleanly merge in @willemarcel's fork, as he has been publishing independently on PyPi. I would like to consolidate the PyPi releases on the master repo.

Wille introduced a CLI application that I am not very familiar with. The tests fail using nosetests. I am not sure how to fix it.

The main change I made is in how you define the response format. Instead of the asGeoJSON boolean parameter, I introduced a responseformat parameter that allows the values xml for OSM XML, json for plain JSON as returned by Overpass, and geojson for GeoJSON output.

GeoJSON response is still the default so this should not break anything.

The only breaking change is that Wille's CLI application now takes those same values for its format parameter.

Please review and see if we can merge this!

willemarcel commented 8 years ago

Hi, @mvexel

I prefer to put the requirements on setup.py, see https://github.com/willemarcel/overpass-api-python-wrapper/blob/master/setup.py

It has the requirements to use and to test the library.

You can install the library to develop and test with

pip install -e .[test]

and run the tests just executing py.test on the terminal

willemarcel commented 8 years ago

Sorry, now I saw the setup.py is correct on this PR. So just try to test with the instructions above. I think everything is OK.

mvexel commented 8 years ago

Hey @willemarcel - this is what I get using nose:

(overpass)mvexel@Martijns-MacBook ~/dev/overpass-api-python-wrapper (example)*$ nosetests
..FF
======================================================================
FAIL: test_cli.test_cli
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/mvexel/.virtualenvs/overpass/lib/python3.5/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/Users/mvexel/dev/overpass-api-python-wrapper/tests/test_cli.py", line 16, in test_cli
    assert result.exit_code == 0
AssertionError:
-------------------- >> begin captured logging << --------------------
requests.packages.urllib3.connectionpool: INFO: Starting new HTTP connection (1): overpass-api.de
requests.packages.urllib3.connectionpool: DEBUG: "POST /api/interpreter HTTP/1.1" 200 322
--------------------- >> end captured logging << ---------------------

======================================================================
FAIL: test_cli.test_cli_xml
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/mvexel/.virtualenvs/overpass/lib/python3.5/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/Users/mvexel/dev/overpass-api-python-wrapper/tests/test_cli.py", line 29, in test_cli_xml
    assert result.exit_code == 0
AssertionError

----------------------------------------------------------------------
Ran 4 tests in 22.513s

FAILED (failures=2)
mvexel commented 8 years ago

and with py.test:

(overpass)mvexel@Martijns-MacBook ~/dev/overpass-api-python-wrapper (example)*$ py.test
===================================================== test session starts =====================================================
platform darwin -- Python 3.5.0, pytest-2.8.5, py-1.4.31, pluggy-0.3.1
rootdir: /Users/mvexel/dev/overpass-api-python-wrapper, inifile:
collected 4 items

tests/test_api.py ..
tests/test_cli.py FF

========================================================== FAILURES ===========================================================
__________________________________________________________ test_cli ___________________________________________________________

    def test_cli():
        runner = CliRunner()
        with runner.isolated_filesystem():
            result = runner.invoke(cli, [
                '--timeout', 40,
                '--endpoint', 'http://overpass-api.de/api/interpreter',
                'node(area:3600362504)[amenity=cafe]', 'out.geojson'
                ])
>           assert result.exit_code == 0
E           assert -1 == 0
E            +  where -1 = <Result NameError("name 'responseformat' is not defined",)>.exit_code

tests/test_cli.py:16: AssertionError
________________________________________________________ test_cli_xml _________________________________________________________

    def test_cli_xml():
        runner = CliRunner()
        with runner.isolated_filesystem():
            result = runner.invoke(cli, [
                '--timeout', 40,
                '--endpoint', 'http://overpass-api.de/api/interpreter',
                '--format', 'osm',
                'node(area:3600362504)[amenity=cafe]', 'out.osm'
                ])
>           assert result.exit_code == 0
E           assert -1 == 0
E            +  where -1 = <Result AttributeError("'list' object has no attribute 'join'",)>.exit_code

tests/test_cli.py:29: AssertionError
============================================= 2 failed, 2 passed in 28.03 seconds =============================================
mvexel commented 8 years ago

also I think the tests need to run faster (collect less data from Overpass perhaps)

mvexel commented 8 years ago

I think I found it. I did change the interface of the CLI application. --format is now --responseformat because format is a reserved word. otherwise I think we're good to go. Comments @itiboi ?

mvexel commented 8 years ago

Hmm, tests pass locally on my machine now, but the last CLI test still fails on travis. Not sure what's going on. @willemarcel any ideas?

mvexel commented 8 years ago

I fixed the tests. I also couldn't resist simplifying the application a bit and getting rid of the output file part. You can just redirect the output to a file yourself as the CLI user, making it more flexible (for piping etc). This all happened in https://github.com/mvexel/overpass-api-python-wrapper/commit/99d390dd59674bfdc372148994b2d26351dd3b25

If you're OK with this I'll merge.

tbolender commented 8 years ago

Just took a look at all the changes, looks good! Are there any plans of a joined development @mvexel and @willemarcel to prevent future divergence of the project?

mvexel commented 8 years ago

@itiboi I think after this merge we should consolidate development here if @willemarcel agrees that is the way to go. This project has really great potential and I think we have a group of good devs here. I propose we cut releases and publish to PyPi from this master repo from now on. I will commit some more time to the project to address issues and coordinate releases. Sounds like a plan?

willemarcel commented 8 years ago

Yes, I agree!

tbolender commented 8 years ago

Great!

mvexel commented 8 years ago

Excellent. Merged!