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

Update diagrams, top-level schema description and schema reference #212

Merged
merged 21 commits into from
Aug 30, 2023

Conversation

duncandewhurst
Copy link
Contributor

@duncandewhurst duncandewhurst commented Aug 28, 2023

Related issues

Description

In addition to updating the diagrams per #178, this PR also:

  • Updates the top-level description in the schema to align with the diagram in docs/rdl/what.md
  • Adds the schema descriptions for dataset, resource and each hazard component to docs/reference/schema.md

You can preview the changes here: https://rdl-standard.readthedocs.io/en/178-diagrams/reference/schema/

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.

@duncandewhurst duncandewhurst marked this pull request as ready for review August 28, 2023 22:23
@duncandewhurst
Copy link
Contributor Author

duncandewhurst commented Aug 28, 2023

Noting that the exposure diagram may need to be updated if there are further changes in #204

Remove empty objects and arrays, make background transparent, add sentence to explain how arrays are represented
@duncandewhurst
Copy link
Contributor Author

Based on @matamadio's feedback in yesterday's call, I've removed empty objects and arrays from the diagrams and added an explanatory sentence to describe how arrays are represented.

Here's how the changes look to the hazard diagram (since it is the most complex):

Before

image

After

image

I considered adding an extra object to each array, but I think it makes the diagram too complicated:

image

Copy link
Contributor

@matamadio matamadio left a comment

Choose a reason for hiding this comment

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

Thanks, they read better now.
Just one perplexity on the word "subset". While formally correct, it can induce to think this is a small part of the whole hazard schema; rather, hazard attributes are a subset of the whole schema. But to be aligned with the rest of the docs, I would use "component" instead. Then for each diagram paragraph I would rephrase as:

The following diagram shows the key attributes of the [hazard] component, with required fields highlighted in blue.

@duncandewhurst
Copy link
Contributor Author

Since @matamadio has approved, I'm going to bypass branch protections and merge this PR so that these changes are available on dev before the workshop today. (Merging was blocked because @stufraser1's earlier review was marked as requesting changes, all of which have been addressed).

@duncandewhurst duncandewhurst merged commit f7cdfbe into dev Aug 30, 2023
@duncandewhurst duncandewhurst deleted the 178-diagrams branch August 30, 2023 20:49
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.

[Docs update] Update or remove mermaid flowcharts
3 participants