-
Notifications
You must be signed in to change notification settings - Fork 6
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
Country and sub-division ISO code validation #112
Country and sub-division ISO code validation #112
Conversation
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.
Excellent stuff. There's an issue with the names
in the counterexamples, but other than that looks good.
counterexamples/admins/locality/invalid-max-length-limit-is-alpha2.json
Outdated
Show resolved
Hide resolved
counterexamples/admins/locality/invalid-max-limit-iso-sub-country-code.json
Outdated
Show resolved
Hide resolved
cc2fc2a
to
0009aac
Compare
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.
I found a second issue in a couple of counter-examples where they include a copy/pasted invalid defaultLanguage
and the extExpectedErrors
is testing the language issue, not the desired issue...
counterexamples/admins/locality/invalid-min-limit-iso-sub-country-code.json
Outdated
Show resolved
Hide resolved
counterexamples/admins/locality/invalid-max-limit-iso-sub-country-code.json
Outdated
Show resolved
Hide resolved
0009aac
to
64835c5
Compare
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.
_,--', _._.--._____
.--.--';_'-.', ";_ _.,-'
.'--'. _.' {`'-;_ .-.>.'
'-:_ ) / `' '=.
) > {_/, /~)
snd |/ `^ .'
🚨 This is behind the schema freeze, so we'll have to wait to merge it to dev. 🚨 |
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.
Approved for me. But would prefer removing min/max length if they're not needed since the pattern already dictates that.
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 — Agree with @jwass, min/max length seems unnecessary if it' always 2 upper-case letters as demanded by the regex.
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.
Approved, would leave the Length fields next to the pattern fields.
counterexamples/admins/locality/invalid-min-limit-iso-sub-country-code.json
Outdated
Show resolved
Hide resolved
counterexamples/admins/locality/invalid-max-length-limit-is-alpha2.json
Outdated
Show resolved
Hide resolved
fd051b2
64835c5
to
fd051b2
Compare
Doing a rebase and merge as this is a well-approved PR. |
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.
Looks good
This PR adds validation to the definitions of
isoCountryCodeAlpha2
andisoSubCountryCode
properties in the admin schema.Issue: https://github.com/OvertureMaps/schema-wg/issues/117
Testing
Tested by modifying examples and creating new counterexamples accordingly and running test script.
Details
The following validation was added to
isoCountryCodeAlpha2
-
"minLength": 2
-
"maxLength": 2
-
"pattern": "^[A-Z]{2}$"
The following validation was added to
isoSubCountryCode
-
"minLength": 4
-
"maxLength": 6
-
"pattern": "^[A-Z]{2}-[A-Z0-9]{1,3}$"
- This validation comes from the ISO reference guide: https://www.iso.org/obp/ui/en/#iso:std:iso:3166:-2:ed-4:v1:en