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 BelgianPostCode #8

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Add BelgianPostCode #8

wants to merge 10 commits into from

Conversation

JDMKSZ
Copy link
Contributor

@JDMKSZ JDMKSZ commented Mar 21, 2024

No description provided.

@JDMKSZ JDMKSZ linked an issue Mar 21, 2024 that may be closed by this pull request
@pvdbosch
Copy link
Contributor

Two remarks:

  • can a description be added? Should usually be aligned with FedVoc.
  • Adding "format: int32" might be useful for code generation (i.e. int instead of long type)

@jpraet
Copy link
Contributor

jpraet commented Mar 22, 2024

I wonder if it shouldn't be BelgianPostalCode?

"post code" (or I find more references to "postcode" as a single word), seems to be British English.
The Belgif guide prescribes American English: https://www.belgif.be/specification/rest/api-guide/#rule-oas-types where they use "postal code" or "ZIP code".

@barthanssens
Copy link

Please do not use "int" for properties that are not meant to be used in calculations... It happens to be that postal codes are (now) all numeric, but no guarantees for the future

And IMHO it's not a good idea to start doing things like "BelgianPostCode", "Belgian.... this, that". A postal code is a string that happens to be valid in a specific country (or more precise, assigned by a postal authority)

@pvdbosch
Copy link
Contributor

@jpraet : postCode was standardized in fedvoc. It was discussed but can't recall exact reason; probably alignment with EU standards.

@barthanssens :

  • it can be useful if an OpenAPI schema of a REST API is more restrictive than the (RDF) definition in the vocabularies: clients want to validate input early and want to know how long values can be (e.g. for display purpose). If the format of the identifier would change, client programs would break even if the restrictions are not documented in the OpenAPI schemas. Making them part of the service contract makes it explicit what the client can depend upon, and allows to track changes.
    • The REST guide does warn against too restrictive schemas (in [id-design] and [oas-enum]), but the Belgian post code's format is quite ossified.
    • In context where international addresses are used, this schema shouldn't be used ofcourse.
  • on string vs integer: this was discussed multiple times at length in the REST guide WG, see here and here, which resulted in guidelines [id-design] and [id-numer] ; which allows for numerical types for existing identifiers

@pvdbosch
Copy link
Contributor

Description is probably be a bit long for display in SwaggerUI (or similar). We can discuss a shorter variant in the functional WG.
Otherwise, schema looks good to me

@pvdbosch
Copy link
Contributor

reviewed and approved by REST WG; still needs review by functional WG

@@ -20,10 +20,21 @@ components:
type: integer
minimum: 10000
maximum: 99999
BelgianPostCode:
description: >
Post code (a.k.a postal code, zip code etc.) of a location in Belgium.
Copy link
Contributor

Choose a reason for hiding this comment

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

My proposition is to just keep the first part:

Post code (a.k.a postal code, zip code etc.) of a location in Belgium.

The rest isn't commonly necessary to understand the field, having it only in fedvoc excel should suffice.
We could add the reference to list of post codes to fedvoc excel in comments column:

A list of Belgian post codes is published by bpost, and is also available as open dataset.

This is modified a bit bc of liberalization of Belgian post market.

will need to review this in functional WG

@pvdbosch
Copy link
Contributor

pvdbosch commented Apr 15, 2024

Functional WG accepted my above proposed changes to the description.

It may be worthwhile to reconsider string for this entry instead of integer to make values be compatible with properties for international post codes, which need to be string.
(Also "0612" has a leading zero, but's that probably negligible)

Then we'd have:

BelgianPostCode:
  description: Post code (a.k.a postal code, zip code etc.) of a location in Belgium
  type: string
  pattern: '^\d{4}$'  # or pattern: '^[1-9]\d{3}$'

@pvdbosch
Copy link
Contributor

for reference, the BeSt search API (https://dtservices.bosa.be/sites/default/files/content/download/files/best_search_openapi.zip) has:

    postCode:
      name: postCode
      in: query
      description: Postal code
      schema:
        type: string
        pattern: '^\d{4}$'
      example: 1000
# ....

      postCode:
        description: Postal code (assigned by bPost)
        type: string
        example: 1500    

So the input format it stricter than the output one.
Note that the example values are invalid: they should have single or double quotes to not be interpreted as a number in YAML.

@JDMKSZ
Copy link
Contributor Author

JDMKSZ commented Apr 16, 2024

Functional WG accepted my above proposed changes to the description.

Description change applied.

It may be worthwhile to reconsider string for this entry instead of integer to make values be compatible with properties for international post codes, which need to be string. (Also "0612" has a leading zero, but's that probably negligible)

I agree. I updated the type according the given proposition. The pattern disallowing leading zeroes ('^[1-9]\d{3}$') seems overly restrictive and not worth the pain. \d{4} is easier to read and it allows 0612 and also 0000 (a value which sometimes occurs in some National Register data). Thorough validation (whether the post code really exists) is up to the business application.

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.

BelgianPostCode
4 participants