Enhancement #2 address details option #58

isikl · 2019-01-23T13:56:45Z

[WP] Add AddressObject() in objects.py

…vice into enhancement-#2-address-details-option

TimMcCauley

@isikl so far so good - we are slowly getting there. Can you implement some logic somewhere around https://github.com/GIScience/openpoiservice/blob/master/openpoiservice/server/db_import/parser.py#L19 where the service checks initially if addresses should be imported? the rest of the logic will depend on the flag set there.

openpoiservice/server/db_import/models.py

openpoiservice/server/db_import/parse_osm.py

openpoiservice/server/db_import/objects.py

isikl · 2019-02-11T17:14:22Z

I guess kind of the problem is the bbox in the database query. Takes coordinates for Heidelberg, but POIs are requested for Bremen, too. Using a large enough bbox manually it works fine, but coudn't figure out yet where the problem arises.

openpoiservice/server/db_import/models.py

openpoiservice/server/db_import/objects.py

openpoiservice/server/db_import/parser.py

isikl · 2019-03-20T11:25:44Z

openpoiservice/tests/test_geocoder.py

+from openpoiservice.server.db_import.objects import AddressObject, GeocoderSetup
+from openpoiservice.server.categories.categories import CategoryTools
+
+# from base import BaseTestCase


Cannot find references 'BaseTestCase'. Same in other testing files. Therefore, line 10 "from openpoiservice.tests.base import BaseTestCase"

The folder structure for testing the osm files is invalid. See "/openpoiservice/tests/base.py#L21"

Corrected space distance

MichaelsJP · 2019-03-26T11:32:27Z

openpoiservice/server/__init__.py

@@ -19,6 +20,13 @@
 basedir = os.path.abspath(os.path.dirname(__file__))
 ops_settings = yaml.safe_load(open(os.path.join(basedir, 'ops_settings.yml')))

+geocoder = None
+geocode_categories = None
+if ops_settings['geocoder'] is not None:


Generally it's better to define those recurring strings in a "static" object somewhere. In this case you could define a static settings variable class in openpoiservice.server and import it alongside the ops_settings object etc.
Another option is that you define all the config file variables you need once in the openpoiservice.server and import these instead of ops_settings.
The reason is, it will reduce complexity and coding time when you have those strings in one place. Once you change something in the config file you'll only have to adjust one variable in the openpoiservice.server file. My advice would be to follow option 2.
E.g.

Solution, define a variable class holding the static variable strings:

>>> class MyConfigVariableClass: ... geocoderSettingsVariable = 'geocoder' ... >>> MyConfigVariableClass.geocoderSettingsVariable 'geocoder'

Solution, define global variables in openpoiservice.server, assign config parameters to it and import them wherever you need them

someSettings = ops_settings['some_config_parameter']['some_sub_config_parameter']

Those strings won't be changed or modified by the user. He will just add further parameters, which will be checked the way you showed. Adjust the strings anyway?

MichaelsJP · 2019-03-26T11:32:50Z

openpoiservice/server/__init__.py

+geocoder = None
+geocode_categories = None
+if ops_settings['geocoder'] is not None:
+    geocoder = GeocoderSetup(list(ops_settings['geocoder'].items())[0]).define_geocoder()


Same here. See above.

MichaelsJP · 2019-03-26T11:49:51Z

openpoiservice/server/api/query_builder.py

@@ -264,12 +256,24 @@ def generate_geojson_features(cls, query, limit):
                }
            properties["category_ids"] = category_ids_obj

+            # Checks if Tags are available
            if q[5][0] is not None:


try-except like in the array call in the following lines?

MichaelsJP · 2019-03-26T11:52:12Z

openpoiservice/server/categories/categories.py

+                        geocode_categories[id] = {}
+
+        return geocode_categories
+


In general it's better to have no nested for-loops. Think about moving every for-loop to a different function...

MichaelsJP · 2019-03-26T11:52:55Z

openpoiservice/server/db_import/objects.py

@@ -14,9 +20,7 @@ def __init__(self, uuid, categories, osmid, lat_lng, osm_type):



Remove dead code?

MichaelsJP · 2019-03-26T13:21:17Z

openpoiservice/server/db_import/objects.py

+    def address_request(self):
+
+        # address_delaied = RateLimiter(self.geocoder.reverse, min_delay_seconds=1)
+        response = self.geocoder.reverse(query=self.lat_lng)


Add a try-except here to catch possible connection errors, e.g. resource not available etc.

MichaelsJP · 2019-03-26T13:23:14Z

openpoiservice/tests/test_geocoder.py

+
+class TestAddressBlueprint(BaseTestCase):
+
+    def test_address_request(self):


Add a test that expects a http connection error to test if it is properly returned by the function (see missing try-except above for address_request)

MichaelsJP · 2019-03-26T13:24:40Z

openpoiservice/server/__init__.py

+geocode_categories = None
+if ops_settings['geocoder'] is not None:
+    geocoder = GeocoderSetup(list(ops_settings['geocoder'].items())[0]).define_geocoder()
+    geocode_categories = CategoryTools('categories.yml').generate_geocode_categories()


Same here. See above

MichaelsJP · 2019-03-26T13:26:07Z

openpoiservice/server/__init__.py

+    geocoder = GeocoderSetup(list(ops_settings['geocoder'].items())[0]).define_geocoder()
+    geocode_categories = CategoryTools('categories.yml').generate_geocode_categories()
+
+
 if "TESTING" in os.environ:


Here you have to decide if you replace the strings. I think this is a one time call so just leave it like that.

MichaelsJP

Top. Good coding style! Here a 🥔 for you...

isikl added 2 commits January 11, 2019 14:21

Merge branch 'development' of https://github.com/GIScience/openpoiser…

4741345

…vice into enhancement-#2-address-details-option

Merge branch 'development' of https://github.com/GIScience/openpoiser…

d096aea

…vice into enhancement-#2-address-details-option

isikl requested a review from TimMcCauley January 23, 2019 13:57

isikl force-pushed the enhancement-#2-address-details-option branch from d48b350 to d096aea Compare January 23, 2019 15:02

isikl added 2 commits January 23, 2019 16:10

[WP] Add geopy geocoding library

d71cab7

[WP] Modified JSON column

ea3a42b

TimMcCauley suggested changes Jan 31, 2019

View reviewed changes

openpoiservice/server/db_import/models.py Outdated Show resolved Hide resolved

openpoiservice/server/db_import/parse_osm.py Outdated Show resolved Hide resolved

openpoiservice/server/db_import/objects.py Outdated Show resolved Hide resolved

[WP] Modified class AddressObject and optional address feature

513def3

TimMcCauley suggested changes Feb 25, 2019

View reviewed changes

[WP] Added class GeocoderSetup(), def generate_geocode_categories()

d155c93

isikl requested a review from MichaelsJP March 15, 2019 16:10

[WP] Added test_geocoder.py

28e4d5b

isikl commented Mar 20, 2019

View reviewed changes

Update __init__.py

44892c0

Corrected space distance

MichaelsJP reviewed Mar 26, 2019

View reviewed changes

MichaelsJP requested changes Mar 26, 2019

View reviewed changes

TimMcCauley force-pushed the development branch 2 times, most recently from 82cf49c to f5995f2 Compare April 26, 2019 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement #2 address details option #58

Enhancement #2 address details option #58

isikl commented Jan 23, 2019

TimMcCauley left a comment

isikl commented Feb 11, 2019

isikl Mar 20, 2019

isikl Mar 20, 2019

MichaelsJP Mar 26, 2019 •

edited

Loading

isikl May 14, 2019

MichaelsJP Mar 26, 2019

isikl May 14, 2019

MichaelsJP Mar 26, 2019

MichaelsJP Mar 26, 2019

MichaelsJP Mar 26, 2019

MichaelsJP Mar 26, 2019

MichaelsJP Mar 26, 2019

MichaelsJP Mar 26, 2019

MichaelsJP Mar 26, 2019

MichaelsJP left a comment

		@@ -14,9 +20,7 @@ def __init__(self, uuid, categories, osmid, lat_lng, osm_type):


		class TestAddressBlueprint(BaseTestCase):

		def test_address_request(self):

Enhancement #2 address details option #58

Are you sure you want to change the base?

Enhancement #2 address details option #58

Conversation

isikl commented Jan 23, 2019

TimMcCauley left a comment

Choose a reason for hiding this comment

isikl commented Feb 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelsJP Mar 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelsJP left a comment

Choose a reason for hiding this comment

MichaelsJP Mar 26, 2019 •

edited

Loading