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

rdls_schema.json: hazards required in event_set, hazard optional in e… #192

Closed
wants to merge 2 commits into from

Conversation

odscjen
Copy link
Contributor

@odscjen odscjen commented Aug 15, 2023

…vent

closes #188

Description

the descriptions of Event_set.hazards and Event.hazard are unchanged as they already specify enough. We'll include more detail about when to include Event.hazard in the documentation

Merge checklist

  • Update the changelog (style guide)
  • Run ./manage.py pre-commit

If you added or removed a field:

  • Update the collapse option of the jsonschema directives for dataset, resource, hazard, exposure, vulnerability and loss on reference/schema.md

Having trouble?

See how to resolve check failures.

@odscjen odscjen requested a review from duncandewhurst August 15, 2023 11:23
@duncandewhurst
Copy link
Contributor

We didn't discuss making Event.hazard optional, my understanding was that we would be leaving it as required. @matamadio please confirm.

@matamadio
Copy link
Contributor

matamadio commented Aug 16, 2023

We didn't discuss making Event.hazard optional, my understanding was that we would be leaving it as required. @matamadio please confirm.

In the case where the whole event_set is consistent in hazard type, it doesn't make too much sense to specify it for each event in the dataset.
In the case where the event_set consists of different hazard types, then it becomes necessary to specify each event's hazard type.

Therefore I''d put it optional, with the instruction to specify it in case of multiple hazard types.

@duncandewhurst
Copy link
Contributor

From a user perspective, I think it's better to always populate Event.hazard even if it is the same for all events. That way, there is only one way to answer the question "what is the hazard type associated with this event?", i.e. check Event.hazard. Otherwise, users and tools need to apply conditional logic, i.e. check Event.hazard, unless it is missing, in which case check Event_set.hazards, which is further complicated by Event_set.hazards being an array.

The downside is that it is slightly more work for publishers to fill Event.hazard with the same value for all events, but not much, e.g. for spreadsheet publication, it's a case of copy-pasting a value down a column.

Therefore, I would avoid instructions like "[only] specify it in case of multiple hazard types."

As for making it optional, if the only reason to have it optional is the case of all events having the same hazard type, I think it should remain as required (notwithstanding my general preference to make as many fields as possible optional!)

@odscjen
Copy link
Contributor Author

odscjen commented Aug 17, 2023

I agree that Event.hazard should be required for the reasons Duncan lays out, @matamadio @stufraser1 let me know if I should update this PR to add it back into the required array or leave it out.

@odscjen
Copy link
Contributor Author

odscjen commented Aug 21, 2023

@matamadio is your thumbs up to the last comment the ok to add hazard back into the required array in Event?

@matamadio
Copy link
Contributor

@matamadio is your thumbs up to the last comment the ok to add hazard back into the required array in Event?

Yes

@odscjen
Copy link
Contributor Author

odscjen commented Aug 21, 2023

Adding that back in means nothing has changed! #181 did some fixes including updating required fields being hazards in event_set and hazard in event so actually this PR can be closed

@odscjen odscjen closed this Aug 21, 2023
@duncandewhurst duncandewhurst deleted the 188-hazard-required branch September 21, 2023 04:09
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.

[Schema] Hazard object (i.e. hazard type and process) required in both event_set and event?
3 participants