-
Notifications
You must be signed in to change notification settings - Fork 23
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 type for SDMX period #107
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.
Just a style note: I'd use CamelCase for schemas.
Co-authored-by: Roberto Polli <[email protected]>
Co-authored-by: Roberto Polli <[email protected]>
Co-authored-by: Roberto Polli <[email protected]>
schemas: | ||
SdmxPeriod: | ||
type: string | ||
pattern: '^\d{4}-?((\d{2}(-\d{2})?)|A1|S[1|2]|Q[1-4]|T[1-3]|M(0[1-9]|1[0-2])|W(0[1-9]|[1-4][0-9]|5[0-3])|D(0[0-9][1-9]|[1-2][0-9][0-9]|3[0-5][0-9]|36[0-6]))?$' |
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.
The pattern is a bit too relaxed, compared to what is allowed by ISO8601. For example, it allows wrong periods like the following:
202012
2020-M12
2020-99
2020-99-99
I understand that covering all ISO8601 with a single regex might be a bit challenging; hence, If we are not seeking an exhaustive validation, then we may proceed with this.
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.
OAS3 allows anyOf
so we can use different patterns.
OAS3 date
and date-time
implies RFC 3339 https://swagger.io/docs/specification/data-models/data-types/
IMHO we can skip all the quirks and limit ISO 8106 to the RFC 3339 profiles.
@ioggstream Since introducing a type for SdmxPeriod, SwaggerHub no longer complains when inputting wrong period formats (like startPeriod=xyz). It can be that I did a mistake somewhere in the spec. but it could also be that the SwaggerHub UI does not support that feature. Do you happen to know the answer? If the UI does not support the feature, I'd be tempted to rollback to the version without a dedicated type for SdmxPeriod. Thanks. |
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.
Ideally, the pattern would cover the complete SDMX time period syntax but I'm not sure this would be possible. I would say, we should go as far as we possibly can.
As proposed by @ioggstream (thanks, @ioggstream!), a type for SDMX periods has been introduced in the Open API definition and is now reused by both start and endPeriod.
Fix #102