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

Coordinates on land #866

Merged
merged 11 commits into from Nov 12, 2018

Conversation

Projects
None yet
3 participants
@shacker
Contributor

shacker commented Nov 6, 2018

What does this changes

  • Creates new geo provider
  • Adds new land_coords provider to geo
  • Moves existing geo_coordinates from address provider to geo provider

What was wrong

Randomly chosen geographical coordinates were most often over ocean.

How this fixes it

Uses an open source data set of existing locations on earth to guarantee that land_coords always provides locations over land (ideal for mapping purposes).

Fixes #857

@shacker

This comment has been minimized.

Contributor

shacker commented Nov 6, 2018

Wanted to add examples to the docs but docs dir in checkout doesn't appear to include actual doc files. Would have added example output:

fake.location_on_land()
>>> ('50.75932', '25.34244', 'Lutsk', 'UA', 'Europe/Kiev')
fake.location_on_land(coords_only=True)
('38.55632', '69.01354')
``

Wanted to add to tests, but faker docs don't mention how to run existing test suite (I'm used to pytest, sorry). 
@fcurella

This comment has been minimized.

Collaborator

fcurella commented Nov 6, 2018

Thank you @shacker

The docs for the provider are automatically generated, so you don't need to add one.

As for tests, there are multiple ways to run them. The easiest is python setup.py test. You can also run them with tox, which will call setup.py test for every supported python version.

@shacker

This comment has been minimized.

Contributor

shacker commented Nov 6, 2018

Ah excellent. I can reproduce the test failure locally now. Give me another day to straighten that out.

@shacker

This comment has been minimized.

Contributor

shacker commented Nov 7, 2018

Mostly going well, but I am a bit stuck. Not with tests for my new provider, but after moving the latitude and longitude tests for existing address.de_AT to geo.de_AT. When running the tests, coordinates are not matching the regex. Here is the mystery: In the new test_geo.py:

class TestDeAT(unittest.TestCase):
    """ Tests in addresses in the de_AT locale """

    def setUp(self):
        self.factory = Faker('de_AT')

    def test_latitude(self):
        latitude = self.factory.latitude()
        import pdb; pdb.set_trace()

And then:

(Pdb) self.factory.locale()
'ms_MY'

I expected locale() to report 'de_AT'. Thought I had an import wrong, but when I search the project for 'ms_MY', I get nothing. Where is this 'ms_MY' coming from?

Thanks for any clues.

@shacker

This comment has been minimized.

Contributor

shacker commented Nov 8, 2018

OK, all tests are green now (though Circle has an unrelated flake8 error). Turns out the cause was that I had set localized = False in the geo provider's init (misunderstood its intent). When you do that and then instantiate a faker like self.factory = Faker('de_AT'), the localization doesn't get picked up -- instead it falls back to ms_MY even though that localization doesn't appear anywhere in the package. Weird!

@@ -108,14 +108,17 @@ def city(self):
def region(self):
return self.random_element(self.regions)
# FIXME: latlng for el_GR should be moved to geo provider in default en_US locale
def latlng(self):

This comment has been minimized.

@shacker

shacker Nov 8, 2018

Contributor

If this is to remain, it should be moved to the top-level geo provider, though its usefulness is dubious since we already have a top-level geo_coord for single coordinates, and now coords_on_land provides a coords_only option which is more useful than this.

This comment has been minimized.

@fcurella

fcurella Nov 8, 2018

Collaborator

i think this should remain for backward-compatibility, but it should call the geo_coord method.

I'd like for it to also raise a PendingDeprecation warning. Ideally, I'd like to manage our deprecations by using the deprecation library. But this could be done in a separate PR

This comment has been minimized.

@shacker

shacker Nov 9, 2018

Contributor

Your second note: "i think this should remain for backward-compatibility, but it should call the geo_coord method" contradicts your first: "Maybe those methods should be called local_latitude and local_longitude or something like that." I've already made the first change, moving them into geo and renaming with local_*. If you feel strongly, I'll undo it and follow your second comment instead. Yes, that means my change is potentially a breaking change for a very small percentage of users, but it's a clean break with less follow-up maintenance hassle in the future and no need to go through a deprecation cycle.

This comment has been minimized.

@fcurella

fcurella Nov 9, 2018

Collaborator

I'd say let's just break compatibility and release the changes under v1.0.0

@fcurella

This comment has been minimized.

Collaborator

fcurella commented Nov 8, 2018

To fix the CircleCI error, rebase your branch against master

@fcurella

This comment has been minimized.

Collaborator

fcurella commented Nov 8, 2018

I think I'd like to have latitude, longitude, geo_coordinate and location_on_land in the main provider, and locale provider will implement local_* methods, which will return points within that country.

I'm still not sure how to work the backward compatibility. Maybe we just don't, and release v1.0.0.

@shacker

This comment has been minimized.

Contributor

shacker commented Nov 9, 2018

I think I'd like to have latitude, longitude, geo_coordinate and location_on_land in the main provider, and locale provider will implement local_* methods, which will return points within that country.

Done!

I'm still not sure how to work the backward compatibility. Maybe we just don't, and release v1.0.0.

It's tricky, and I'm not sure either, but I think this will affect very few users and you might be better off just adding it to release notes and rolling forward.

@shacker

This comment has been minimized.

Contributor

shacker commented Nov 9, 2018

Out of curiosity, why are you running CI on all of appveyor, travis, and circle? Aren't they all doing the same thing / isn't one of them enough? Appveyor and Travis are taking forever tonight.

@kungfu71186

This comment has been minimized.

Contributor

kungfu71186 commented Nov 9, 2018

@shacker windows and linux

@shacker

This comment has been minimized.

Contributor

shacker commented Nov 9, 2018

Yeesh. 30 seconds locally, 12 minutes+ on Travis...

screenshot 2018-11-08 23 51 54

@kungfu71186

This comment has been minimized.

Contributor

kungfu71186 commented Nov 9, 2018

@shacker looks like its due to the fact it has to install pypy from clean build. Appears to still be an open ticket :/

travis-ci/travis-ci#9542

from .. import Provider as AddressProvider
def contains_point(poly, point):

This comment has been minimized.

@shacker

shacker Nov 9, 2018

Contributor

This was unused.

)
def local_latlng(self):
return float(self.local_latitude()), float(self.local_longitude())

This comment has been minimized.

@fcurella

fcurella Nov 9, 2018

Collaborator

I'm thinking we could move this one method to the superclass in faker/providers/geo/__init__.py. I think it would be the same implementation for most localized providers.

This comment has been minimized.

@shacker

shacker Nov 10, 2018

Contributor

Thinking through this and experimenting, I'm not sure this can work. local_latlng depends on the presence of local_latitude and local_longitude. But what does "local" mean in the context of a non-localized provider? The only option would be to have them provide random coords, with a docstring suggesting that you implement a localized override for your locale. So I could make it work, but it would be meaningless, since "local_foo" wouldn't be local at all (unless we do IP detection and lookup!)

The only option I can think of that would really work would be to make it take a required argument country_code=XY and then query the land_coords list for tuples matching that country code. Not sure how performant that would be, but it could work. Let me know if you want me tackle this (or if you want me to make local_* return random coords if not overridden).

This comment has been minimized.

@shacker

shacker Nov 10, 2018

Contributor

Querying the land_coords list for a global local_latlng with required country code seems to be very fast, and not a performance concern. I've pushed another commit implementing this idea, but can undo it if you don't approve.

The odd thing about this is that it depends on passing two-letter country codes ('US'), rather than five-character ('en_US') locales used elsewhere in Faker. Is this a problem? It makes sense because we're interested here in a country, not a language.

This comment has been minimized.

@fcurella

fcurella Nov 12, 2018

Collaborator

I think it makes sense for most countries, but not necessarily for all. Some countries have administrative subdivisions with different official languages. Switzerland's Cantons and Canada's Provinces come to mind.

I don't think we need to implement those right away. I'd rather leave it to people living in those countries, as they know what's more appropriate. But I think we need to make it possible to support that use case.

This comment has been minimized.

@shacker

shacker Nov 12, 2018

Contributor

with different official languages

To be clear, this feature is language-independent. You pass in a two-letter country code (not a five-char language identifier) and you get back a pair of coordinates in that country. e.g. fake.local_latlng('AU'). And of course indpendent locales can override the implementation.

That said, do you accept the approach of the last commit? And if so, is there anything else for me to do here?

This comment has been minimized.

@fcurella

fcurella Nov 12, 2018

Collaborator

I think this is good enough for now :) Thank you so much for all the hard work!

This comment has been minimized.

@shacker

shacker Nov 12, 2018

Contributor

Excellent, thank you!

@fcurella fcurella merged commit afb59ba into joke2k:master Nov 12, 2018

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 96.245%
Details

fcurella added a commit that referenced this pull request Nov 13, 2018

[BREAKING] Coordinates on land (#866)
* Add land_coords provider

* Move existing geo_coordinate methods to geo provider

* Fix latitude and longitude tests after moving to geo provider

* Move Greek lat/long providers to geo.local_*

* Add tests for all providers now in geo; rename geo_coordinate to coordinate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment