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

rdl_schema_0_1.json: update exposure object #101

Merged
merged 36 commits into from
Jul 4, 2023
Merged

Conversation

odscjen
Copy link
Contributor

@odscjen odscjen commented Jun 23, 2023

Resolves #62, adds new object in $defs for Cost which will be reused in Vulnerability and Loss.

References Temporal object but this hasn't been created yet.

@odscjen odscjen requested a review from duncandewhurst June 23, 2023 14:49
@odscjen odscjen mentioned this pull request Jun 26, 2023
Copy link
Contributor

@duncandewhurst duncandewhurst left a comment

Choose a reason for hiding this comment

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

Need to do the following steps described in #105 (review):

Let's merge #109 and #110 before merging this PR. To reduce the number of merge conflicts, once those PRs are merged, we can reindent the schema files in this PR by running ocdskit indent -r . and then merge dev into this PR.

Let's also run OCDS Kit's schema-strict command to add validation properties to prevent empty arrays, strings and objects.

schema/rdl_schema_0.1.json Outdated Show resolved Hide resolved
schema/rdl_schema_0.1.json Outdated Show resolved Hide resolved
schema/rdl_schema_0.1.json Outdated Show resolved Hide resolved
schema/rdl_schema_0.1.json Outdated Show resolved Hide resolved
schema/rdl_schema_0.1.json Outdated Show resolved Hide resolved
schema/rdl_schema_0.1.json Outdated Show resolved Hide resolved
schema/rdl_schema_0.1.json Outdated Show resolved Hide resolved
schema/rdl_schema_0.1.json Outdated Show resolved Hide resolved
schema/rdl_schema_0.1.json Outdated Show resolved Hide resolved
specs/code-lists/CSV/cost_type.csv Outdated Show resolved Hide resolved
@odscjen
Copy link
Contributor Author

odscjen commented Jun 28, 2023

The error it's finding is just that Temporal is referenced by it's not been created yet, it's in other PR

@duncandewhurst
Copy link
Contributor

When adding codelist links in schema descriptions, please use the version placeholders introduced in #10, e.g. https://rdl-standard.readthedocs.io/en/{{version}}/data_model/codelists#risk_data_type

@duncandewhurst
Copy link
Contributor

The tests expect definitions that appear in an array property's items to have an id field. That isn't always necessary, for example, when the definition has no array properties itself so we could add an exception for this case.

However, given that Cost.unit is always a currency and it is always possible to convert between currencies, I don't see the value of including that field so we could just remove the Cost definition and make Exposure.cost a string array using the cost_type.csv codelist. Sound good?

Regarding the other error, see my comment in #105 (comment) - I think the same thing has happened here.

@duncandewhurst
Copy link
Contributor

duncandewhurst commented Jun 29, 2023

Regarding the other error, see my comment in #105 (comment) - I think the same thing has happened here.

Having looked into it more, the issue is that the common_exp_category definition was removed in favour of Exposure.category referencing the exposure_category codelist, per my suggestion in #101 (comment).

To avoid the schema being in an incoherent state we should either reinstate the common_exp_category definition until the other places that reference it are updated or we should update those other places to reference the exposure_category codelist as a part of this PR.

Copy link
Contributor

@duncandewhurst duncandewhurst left a comment

Choose a reason for hiding this comment

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

Need to run ./manage.py pre-commit to update docs/data_model/codelists.md.

schema/rdl_schema_0.1.json Outdated Show resolved Hide resolved
schema/rdl_schema_0.1.json Outdated Show resolved Hide resolved
@odscjen
Copy link
Contributor Author

odscjen commented Jun 30, 2023

However, given that Cost.unit is always a currency and it is always possible to convert between currencies, I don't see the value of including that field so we could just remove the Cost definition and make Exposure.cost a string array using the cost_type.csv codelist. Sound good?

I can imagine a case where there are costs for 2 types in different currencies though, in that case I think currency still needs to be provided? @stufraser1 thoughts?

@duncandewhurst
Copy link
Contributor

However, given that Cost.unit is always a currency and it is always possible to convert between currencies, I don't see the value of including that field so we could just remove the Cost definition and make Exposure.cost a string array using the cost_type.csv codelist. Sound good?

I can imagine a case where there are costs for 2 types in different currencies though, in that case I think currency still needs to be provided? @stufraser1 thoughts?

In the case of a dataset containing costs in multiple currencies, even if the currencies were specified in the RDLS metadata, the currencies for each cost would also need to be specified in the dataset itself otherwise it would be impossible to know which cost was in which currency,

In the case of a dataset containing costs in a single currency, I had assumed that currencies would also be specified within the dataset itself so. If that's not the case, then there is an argument for putting currency in the metadata.

@odscjen
Copy link
Contributor Author

odscjen commented Jul 3, 2023

I think that both cost.type and cost.unit are needed so I'll add in an id field which will
a) future proof the schema incase any future objects need to refer to a cost item
b) make flattening easier
c) make the test pass

…sudo),29(audio),30(dip),46(plugdev),107(input),120(lpadmin),132(lxd),133(sambashare),30000(clevo-keyboard) to and remove references to common_exp_val_type
@odscjen odscjen requested review from stufraser1 and matamadio July 3, 2023 09:50
matamadio
matamadio previously approved these changes Jul 4, 2023
@odscjen odscjen dismissed duncandewhurst’s stale review July 4, 2023 14:07

requested changes applied

@odscjen odscjen requested a review from odscrachel July 4, 2023 14:30
@odscjen odscjen merged commit 67cb23b into dev Jul 4, 2023
@odscjen odscjen deleted the 62_exposure_schema branch July 4, 2023 14:38
@stufraser1
Copy link
Member

In the case of a dataset containing costs in multiple currencies, even if the currencies were specified in the RDLS metadata, the currencies for each cost would also need to be specified in the dataset itself otherwise it would be impossible to know which cost was in which currency,

I agree. Usual practice to include currency code in the data.

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.

[Proposal] Exposure component
5 participants