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

Publication formats #90

Merged
merged 15 commits into from
Sep 14, 2022
Merged

Publication formats #90

merged 15 commits into from
Sep 14, 2022

Conversation

duncandewhurst
Copy link
Collaborator

@duncandewhurst duncandewhurst commented Sep 13, 2022

Related to #51, #75, #82 and #37

This PR integrates the content from the proposal in #51 to the publication formats reference, adds guidance on publishing large networks in JSON format, based on the proposal in #75 and adds general guidance on publication formats.

@lgs85 contrary to our discussion about separating the packaging reference from the publication formats reference, I realised it would be simpler to integrate them and require that data is published using one of the packaging/publication formats, which are now one and the same. Otherwise, it gets very confusing to have both publication formats and packaging formats.

I struggled to integrate the guidance on paginating/streaming nodes and links to the reference on paginating/streaming whole networks, so it is currently sat in the guidance, but we might want to move it to the reference if you can think of a good way of integrating it.

Copy link
Contributor

@lgs85 lgs85 left a comment

Choose a reason for hiding this comment

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

Looks good. I agree that the split across guidance and reference isn't ideal, but let's get something in then sort. A few suggested edits and questions in here, then I think we can merge.

docs/guidance/publication.md Outdated Show resolved Hide resolved
To convert a network to GeoJSON format:

* Check out the [repository](https://github.com/Open-Telecoms-Data/open-fibre-data-standard)
* Run the [get started commands](https://ofds-standard-development-handbook.readthedocs.io/en/latest/technical/build.html#get-started)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Run the [get started commands](https://ofds-standard-development-handbook.readthedocs.io/en/latest/technical/build.html#get-started)
* Run the [get started commands](https://ofds-standard-development-handbook.readthedocs.io/en/latest/technical/build.html#get-started) in the OFDS developer handbook.

This is not ideal, as it blurs the boundary between publisher and developer documentation. Might it better to replicate the getting started instructions here until we have a standalone tools?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 3044614


To convert a network to GeoJSON format:

* Check out the [repository](https://github.com/Open-Telecoms-Data/open-fibre-data-standard)
Copy link
Contributor

Choose a reason for hiding this comment

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

Clone the repository?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 048dbff

docs/guidance/publication.md Outdated Show resolved Hide resolved
docs/guidance/publication.md Outdated Show resolved Hide resolved
docs/reference/publication_formats.md Outdated Show resolved Hide resolved
docs/reference/publication_formats.md Outdated Show resolved Hide resolved

:::{tab-item} Small file example
The following example shows a network package containing two networks:
```{jsoninclude} ../../examples/json/network-package.json
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be good to add a few example fields here, clarity

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 2b4ae25

The following example shows a JSON Lines file containing two networks:

```
{"id": "1"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Again I think a few more example fields would be useful here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 2b4ae25

The following example shows a newline-delimited GeoJSON file containing two features:

```
{"type": "Feature"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a few more example fields here too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 2b4ae25

@duncandewhurst
Copy link
Collaborator Author

@lgs85 this is ready for you re-review and merge. I've also run ocdskit schema-strict per #84

@lgs85 lgs85 merged commit bfcea74 into main Sep 14, 2022
@lgs85 lgs85 deleted the publication-formats branch September 14, 2022 08:02
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.

2 participants