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

Inventory classification change - fix birthPeriod validation #334

Merged
merged 2 commits into from
Oct 6, 2022

Conversation

cookeac
Copy link
Collaborator

@cookeac cookeac commented Oct 4, 2022

birthPeriod was previously specified as an icarDateType, which is half-right, because it is a date range.
Most validators interpret "format": "date" as an RFC3339 date, which does not allow for ranges.

Instead I have changed its type to string, and defined a pattern with a regex. If this is too complex we can ditch the regex and simply make it a string with the current description.

InventoryClassification birth period previously specified the birth period as an (RFC3339) icarDateType. But in fact, per the description it is an ISO8601 interval.
Changed the type to string, and added a pattern which matches a simple ISO8601 date interval (no time component) and the year and month options as per the description.
@cookeac cookeac changed the base branch from ADE-1 to Develop October 4, 2022 01:39
Copy link
Collaborator

@erwinspeybroeck erwinspeybroeck left a comment

Choose a reason for hiding this comment

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

No comment from my side

Copy link
Collaborator

@AndreasSchultzGEA AndreasSchultzGEA left a comment

Choose a reason for hiding this comment

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

  • the 2 parts differ. whereas the first one is very flexible, the second one is fixed to YYYY-MM-DDZ
  • the first part offers the possibility to insert YYYY-99, e.g. by defining [0-9]{2} as month
  • Is the second part really optional? I think not. The separator as well.

@cookeac
Copy link
Collaborator Author

cookeac commented Oct 5, 2022

Having tested this in https://regex101.com/ there are three effective captures:

  • ([0-9]{4}) - four digits (anything from 0000 to 9999 but effectively designed as year)
  • ([0-9]{4}-[0-9]{2}) - four digits, a dash, and 2 digits. Intended to represent YYYY-MM but as you say it does allow for 9999-99. I will make a change to allow 00 to 19 for months (because more than that makes the regex complex).
  • ([0-9]{4}-[0-9]{2}-[0-9]{1,2}(Z?)(\/|--)[0-9]{4}-[0-9]{2}-[0-9]{1,2}(Z?))) - intended to be YYYY-MM-DDZ/YYYY-MM-DDZ
    The Zs are optional (indicated by the question mark after them), but we could remove them completely. The only optionality then is whether you have a / or -- symbol. The latter is an alternative more suited to some operating systems.

image

So I think the optionality is right, but would be willing to accept corrections :-)
The remaining questions are:

  • Do we even want the Z given that we just have dates and not times? I will remove if not.
  • Do we want more sophisticated regex that enforces the exact day and month syntax? Otherwise I will just make changes that get us in the right approximate range - for instance 00-19 month and 00-39 days. This is easy.

Tightened the regex to make it clearer that dates are intended (as per description) but this is not perfect, its just simple and fast.
@cookeac cookeac merged commit af62117 into adewg:Develop Oct 6, 2022
@cookeac cookeac deleted the inventory-classification-changes branch October 6, 2022 06:53
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.

Birth Period in icarInventoryClassificationType doesn't validate as per description
3 participants