Skip to content
This repository was archived by the owner on Jun 10, 2025. It is now read-only.

Merge Wille's fork and improvements #36

Merged
merged 12 commits into from
Dec 16, 2015
Merged

Merge Wille's fork and improvements #36

merged 12 commits into from
Dec 16, 2015

Conversation

mvexel
Copy link
Owner

@mvexel mvexel commented Dec 12, 2015

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
Copy link

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
Copy link

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
Copy link
Owner Author

mvexel commented Dec 14, 2015

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
Copy link
Owner Author

mvexel commented Dec 14, 2015

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
Copy link
Owner Author

mvexel commented Dec 14, 2015

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

@mvexel
Copy link
Owner Author

mvexel commented Dec 14, 2015

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
Copy link
Owner Author

mvexel commented Dec 14, 2015

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
Copy link
Owner Author

mvexel commented Dec 15, 2015

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 99d390d

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

@tbolender
Copy link
Collaborator

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
Copy link
Owner Author

mvexel commented Dec 15, 2015

@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
Copy link

Yes, I agree!

@tbolender
Copy link
Collaborator

Great!

mvexel added a commit that referenced this pull request Dec 16, 2015
Merge Wille's fork and improvements
@mvexel mvexel merged commit ce19e14 into master Dec 16, 2015
@mvexel
Copy link
Owner Author

mvexel commented Dec 16, 2015

Excellent. Merged!

@mvexel mvexel deleted the merge-wille branch December 16, 2015 01:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants