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

Add country code to incident metadata #3169

Merged
merged 5 commits into from
Jun 28, 2021
Merged

Conversation

mandeepsandhu
Copy link
Contributor

@mandeepsandhu mandeepsandhu commented Jun 24, 2021

Added country code (iso 2 & 3 char) to incident metadata.

@@ -85,7 +85,8 @@ def make_request(post_body):
parser.add_argument('--output-dir', type=str, help='The directory in which to place the result of each request')
parser.add_argument('--concurrency', type=int, help='The number of processes to use to make requests', default=multiprocessing.cpu_count())
parser.add_argument('--format', type=str, help='Supports csv, json, raw and null output formats', default='csv')
parser.add_argument('--headers', type=str, help='Additional http headers to send with the requests. Follows the http header spec, eg. some-header-name: some-header-value', action='append', nargs='*')
parser.add_argument('--headers', type=str, help='Additional http headers to send with the requests. Follows the http\
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This accidentally crept in. This script is currently broken in master. But I remember a PR from either @kevinkreiser or @merkispavel that fixed it (can't find the PR right now though :/ )

Copy link
Member

Choose a reason for hiding this comment

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

its over in @genadz 's pr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah ok 🤦

auto country_iso3 = iso2_to_iso3.find(admin.country_code());
if (country_iso3 != iso2_to_iso3.end()) {
admin_map->emplace("iso_3166_1_alpha3", country_iso3->second);
auto country_iso3 = valhalla::tyr::get_iso_3166_1_alpha3(admin.country_code());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the lookup to a util function.

src/tyr/serializers.cc Outdated Show resolved Hide resolved
{"DEF", {{"highway", "primary"}}}, {"GHI", {{"highway", "primary"}}},
{"ADG", {{"highway", "motorway"}}}, {"BE", {{"highway", "secondary"}}},
{"EH", {{"highway", "tertiary"}}},
{"AB", {{"highway", "tertiary"}, {"driving_side", "left"}}},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to add driving side since we're no longer using admin db and instead manually setting certain admin attributes.

src/thor/triplegbuilder.cc Outdated Show resolved Hide resolved
src/thor/triplegbuilder.cc Outdated Show resolved Hide resolved
valhalla/tyr/util.h Outdated Show resolved Hide resolved
@mandeepsandhu
Copy link
Contributor Author

@kevinkreiser @purew this is ready for review. gist of changes:

  • moved the iso 2->3 conversion function to baldr/admin (i felt admin was the appropriate place as the iso lookup is kind of related)
  • use endnode for incident CC
  • updated tests

purew
purew previously approved these changes Jun 28, 2021
Copy link
Contributor

@purew purew left a comment

Choose a reason for hiding this comment

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

LGTM!

Added country code (iso 2 & 3 char) to incident metadata. Also renamed
the existing country code attribute in incident metadata from
iso_3166_1_alpha2 to iso_3166_1 to be consistent with similar attribute
when serializing admins in route response.
@mandeepsandhu mandeepsandhu merged commit 8122861 into master Jun 28, 2021
@purew purew deleted the incident_country_code branch June 28, 2021 21:07
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

3 participants