-
Notifications
You must be signed in to change notification settings - Fork 431
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 fields country_name
and region_iso_code
to geo
.
#214
Conversation
Ping @KPStolk, @ruflin and @MikePaquette |
@@ -49,3 +49,17 @@ | |||
description: > | |||
City name. | |||
example: Montreal | |||
|
|||
- name: country_iso_code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just call it country
and have iso_code
part of the description? Same below.
Main problem: Breaks with the generic geo processing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand what you mean by "Breaks with the generic geo processing". This is the field name as it is currently output by our geoip processor.
You're suggesting we use the canonical word ("country") for the field meant to contain the ISO code? If we were to use the canonical word for something, I'd rather it be the human name instead (country:Canada
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, what we have in here works. It just feels like the names are too long.
Let's move forward with it and figure it out later :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
2ba647a
to
16c2bd6
Compare
Also sort the fields in the set by data format and by size: location, human names,
and ISO codes :-)
Closes #177.