Added parsing of county #51

boldtrn · 2018-06-19T13:48:15Z

Fixes #50.

This PR adds the ability to parse the county field of Nominatim, OCD, and in theory for Gisgraphy. The Gisgraphy part is untested.

boldtrn · 2018-06-19T13:49:30Z

src/test/resources/converter.yml

@@ -1 +1,5 @@
 nominatimURL: https://nominatim.openstreetmap.org/search/
+
+openCageDataURL: https://api.opencagedata.com/geocode/v1/json
+openCageDataKey: XXXXXXXX


I could in theory commit my personal ocd key here, it's a free key and is pretty limited anyway, but that way we could actually test ocd in travis. Like this the test will always fail.

WDYT @karussell?

Ok, let's try. Or we can also try to hide it and then it will work on travis only?

Ok sure, one downside of this is that we cannot run tests for any PRs if the env variable is encrypted, because travis does not share encrypted variables with forked repos. So maybe every contributor needs to set up their own keys or it's not possible at all (not sure yet).

This should not be a problem for now.

boldtrn · 2018-06-19T13:51:54Z

src/main/java/com/graphhopper/converter/api/GisgraphySearchEntry.java

@@ -35,6 +35,9 @@
    @JsonProperty("adm1_name")
    private String adm1Name;

+    @JsonProperty("adm3_name")


I wonder that the @JsonProperty also for the other properties in this class work. For example see this request. The fields in json are named: adm3Name or zipCode. But as I mentioned, I wasn't able to test Gisgraphy (see #48). I just followed the existing scheme here.

boldtrn · 2018-06-20T09:20:30Z

src/main/java/com/graphhopper/converter/core/Converter.java

@@ -16,7 +16,7 @@ public static GHEntry convertFromGisgraphyAddress(GisgraphyAddressEntry gisgraph
        GHEntry rsp = new GHEntry(null, null, gisgraphyEntry.getLat(),
                gisgraphyEntry.getLng(), gisgraphyEntry.getDisplayName(), null,
                gisgraphyEntry.getCountry(), gisgraphyEntry.getCity(),
-                gisgraphyEntry.getState(), gisgraphyEntry.getStreetName(),
+                gisgraphyEntry.getState(),  gisgraphyEntry.getAdm3Name(), gisgraphyEntry.getStreetName(),


When thinking about this a bit more, I am not sure if we should do this. We convert Adm3Name to county, but maybe this is not true for other regions of the world? Gisgraphy does not return county, maybe we just return null then instead?

Yes, admin3 is not always county. Do you plan to add county @gisgraphy ?

gisgraphy · 2018-06-20T10:56:14Z

County is very country specific and can not be general field. As you can see on this page https://wiki.openstreetmap.org/wiki/Tag:boundary=administrative

We prefer administrative division with level. We import several datasets (tiger next month) and it seems to be a good choice

gisgraphy · 2018-06-20T11:13:23Z

If you know in which country there is a county you may map the adm field to county.

boldtrn · 2018-06-21T11:27:52Z

Thanks for the information @gisgraphy.

@karussell, so I would remove the county parsing from Gisgraphy for now?

karussell · 2018-06-21T11:28:35Z

Ok

boldtrn · 2018-06-26T09:35:22Z

Ok, I removed the Gisgraphy Adm3 part again.

boldtrn · 2018-06-27T07:28:13Z

BTW: I set the gisgraphy tests to ignore for now, so that the tests can run successfully for the other providers.

boldtrn · 2018-06-27T07:31:47Z

src/main/java/com/graphhopper/converter/resources/ConverterResourceOpenCageData.java

@@ -41,17 +41,19 @@ public Response handle(@QueryParam("q") @DefaultValue("") String query,
                           @QueryParam("nominatim") @DefaultValue("false") boolean nominatim,
                           @QueryParam("find_osm_id") @DefaultValue("true") boolean find_osm_id,
                           @QueryParam("reverse") @DefaultValue("false") boolean reverse,
-                           @QueryParam("point") @DefaultValue("false") String point
+                           @QueryParam("point") @DefaultValue("false") String point,
+                           @QueryParam("ocd_key") @DefaultValue("") String ocdKey


Allowing to provide an ocd_key per request seemed the cleanest solution to inject the key from an environment variable. Another solution could have been to check if there is an environment variable in ConverterApplication. Let me know if I should move the code there. For now I decided against it, because it's too much "magic" for my taste. When debugging an issue you would easily miss this and wonder why it works locally but not on the server.

Will this work?

https://stackoverflow.com/a/32231630/194609
https://www.dropwizard.io/1.3.0/docs/manual/core.html#environment-variables

Ah, good point! This works great. BTW: We could also use the same approach for Gisgraphy and specify private environment variables for this as well?

can I remove the user-agent that allow requests (for tests) on #48 ?

If I understand Travis and other will use an env variable as the API key, right ?

If I understand Travis and other will use an env variable as the API key, right ?

Yes, that is correct. I think you can remove the user agent :).

Added parsing of county

90d735e

boldtrn added the enhancement label Jun 19, 2018

boldtrn commented Jun 19, 2018

View reviewed changes

Fix gisgraphy

be13b65

boldtrn commented Jun 20, 2018

View reviewed changes

boldtrn requested a review from karussell June 20, 2018 09:20

Removed ADM3 parsing for Gisgraphy

574d6ee

Add Environment Variable support to OCD tests

ee55169

boldtrn commented Jun 27, 2018

View reviewed changes

boldtrn added 2 commits July 2, 2018 16:36

Use environment variable in config

251dd94

Use environment variable for gisgraphy as well

8a4af4b

karussell approved these changes Jul 3, 2018

View reviewed changes

boldtrn merged commit 0471d29 into master Jul 3, 2018

boldtrn deleted the issue_50 branch July 3, 2018 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added parsing of county #51

Added parsing of county #51

boldtrn commented Jun 19, 2018

boldtrn Jun 19, 2018

boldtrn Jun 26, 2018

karussell Jun 26, 2018

boldtrn Jun 27, 2018

karussell Jun 27, 2018

boldtrn Jun 19, 2018 •

edited

Loading

boldtrn Jun 20, 2018

karussell Jun 20, 2018

gisgraphy commented Jun 20, 2018

gisgraphy commented Jun 20, 2018

boldtrn commented Jun 21, 2018

karussell commented Jun 21, 2018

boldtrn commented Jun 26, 2018

boldtrn commented Jun 27, 2018

boldtrn Jun 27, 2018

karussell Jun 27, 2018

boldtrn Jul 2, 2018

karussell Jul 2, 2018

gisgraphy Jul 3, 2018

boldtrn Jul 3, 2018

gisgraphy Jul 3, 2018

Added parsing of county #51

Added parsing of county #51

Conversation

boldtrn commented Jun 19, 2018

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

boldtrn Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gisgraphy commented Jun 20, 2018

gisgraphy commented Jun 20, 2018

boldtrn commented Jun 21, 2018

karussell commented Jun 21, 2018

boldtrn commented Jun 26, 2018

boldtrn commented Jun 27, 2018

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

boldtrn Jun 19, 2018 •

edited

Loading