Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests & encoding fixing #342

Merged
merged 11 commits into from Mar 13, 2018
Merged

Tests & encoding fixing #342

merged 11 commits into from Mar 13, 2018

Conversation

thomas-lab
Copy link
Collaborator

Python 2 encoding issues are a living hell. Tried to do the best out of it, and it works well! 🙃
Thanks @ebreton for detecting these issues again!

& bumped the version number

@ebreton
Copy link
Collaborator

ebreton commented Mar 2, 2018

@ThmsLa you are more than welcome !

My pull request was pending in the air and started to rot a bit (some conflicts emerged). I therefore merged it, and that generated some conflicts in your branch which I also try to resolve.

Some tests are failing (among them mapzen which we has nothing to do with your PR)... I will have a look at it tomorrow. Bedtime here :)

@ebreton ebreton mentioned this pull request Mar 3, 2018
@ebreton
Copy link
Collaborator

ebreton commented Mar 3, 2018

Hi @ThmsLa ,

Please look at #343 for a few suggestions on refactoring (you have address somethings in your PR already).

Withor without my suggestions, the objective is to make the bing tests pass 😎

@thomas-lab
Copy link
Collaborator Author

Thanks @ebreton!
Sorry I messed up your tests with my old commits. Bing tests are now running fine.

The MapQuest test failing seems to be linked to some strange API key error. MapZen's have an SSL error, I guess we can't do much about it.

@thomas-lab
Copy link
Collaborator Author

thomas-lab commented Mar 6, 2018

Regarding the MapQuest test, it seems linked to paid API keys having errors issues sometimes: https://developer.mapquest.com/forum/paid-key-intermittent-403-error-not-valid-key

... and these tests work with my free key locally. Have you tried using another key? :)

@DenisCarriere
Copy link
Owner

@ThmsLa Not too sure if this PR is relevant anymore?

@thomas-lab
Copy link
Collaborator Author

@DenisCarriere I'll check next Monday, but a few things might be relevant, especially regarding the tests. Thanks for the invitation btw! :)

@thomas-lab thomas-lab changed the title Bing encoding issues fixed! Tests & encoding fixing Mar 12, 2018
@codecov-io
Copy link

codecov-io commented Mar 12, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@34560a1). Click here to learn what that means.
The diff coverage is 59.45%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #342   +/-   ##
=========================================
  Coverage          ?   84.97%           
=========================================
  Files             ?       66           
  Lines             ?     3728           
  Branches          ?        0           
=========================================
  Hits              ?     3168           
  Misses            ?      560           
  Partials          ?        0
Impacted Files Coverage Δ
geocoder/google.py 76.86% <0%> (ø)
geocoder/mapzen.py 62.5% <100%> (ø)
geocoder/mapquest_batch.py 95.45% <100%> (ø)
geocoder/bing_batch_reverse.py 63.79% <37.5%> (ø)
geocoder/bing_batch.py 76.08% <75%> (ø)
geocoder/bing_batch_forward.py 82.6% <75.75%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34560a1...258246e. Read the comment docs.

@thomas-lab
Copy link
Collaborator Author

@ebreton @DenisCarriere Since MapZen is shutting down, I marked the MapZen class as deprecated and updated the tests accordingly.
Seems better than removing all the MapZen related files, as users using it will get a relevant exception from it. If you're fine with it I'll merge :)

@DenisCarriere
Copy link
Owner

👍 Nice! Agreed, we can keep Mapzen but define it as deprecated 😢

Amazing, the first PR that has passing tests ✅ 🚀

@thomas-lab thomas-lab merged commit 48bc43a into DenisCarriere:master Mar 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants