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

Added parsing of county #51

Merged
merged 6 commits into from
Jul 3, 2018
Merged

Added parsing of county #51

merged 6 commits into from
Jul 3, 2018

Conversation

boldtrn
Copy link
Member

@boldtrn boldtrn commented Jun 19, 2018

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.

@@ -1 +1,5 @@
nominatimURL: https://nominatim.openstreetmap.org/search/

openCageDataURL: https://api.opencagedata.com/geocode/v1/json
openCageDataKey: XXXXXXXX
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT @karussell?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be a problem for now.

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

@JsonProperty("adm3_name")
Copy link
Member Author

@boldtrn boldtrn Jun 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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(),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@boldtrn boldtrn requested a review from karussell June 20, 2018 09:20
@gisgraphy
Copy link
Contributor

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

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

@boldtrn
Copy link
Member Author

boldtrn commented Jun 21, 2018

Thanks for the information @gisgraphy.

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

@karussell
Copy link
Member

Ok

@boldtrn
Copy link
Member Author

boldtrn commented Jun 26, 2018

Ok, I removed the Gisgraphy Adm3 part again.

@boldtrn
Copy link
Member Author

boldtrn commented Jun 27, 2018

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, sure

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done :)

@boldtrn boldtrn merged commit 0471d29 into master Jul 3, 2018
@boldtrn boldtrn deleted the issue_50 branch July 3, 2018 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants