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

GeoJSON -> Json: Include name in phase and org references #33

Closed
odscjames opened this issue Nov 10, 2022 · 7 comments
Closed

GeoJSON -> Json: Include name in phase and org references #33

odscjames opened this issue Nov 10, 2022 · 7 comments

Comments

@odscjames
Copy link
Collaborator

Wasn't sure about this and kept meaning to ask as either is technically valid and there isn't an equivalent of https://open-fibre-data-standard.readthedocs.io/en/0.1-dev/reference/publication_formats/geojson.html?highlight=geojson#geojson-transformation-specification to follow (should there be?)

But Open-Telecoms-Data/cove-ofds#5 (comment) says:

.name is missing from all the phase references.
.name is missing from all the organisation references

@duncandewhurst
Copy link
Contributor

Yes, let's include .name.

As a principle, I think that we should aim for GeoJSON/JSON to be round-trippable, like the CSV format, i.e. after converting a file from JSON to GeoJSON, if you convert it back again, you should get the same file you started with.

I've opened an issue on documenting a specification for the GeoJSON to JSON transformation: Open-Telecoms-Data/open-fibre-data-standard#185

@odscjames
Copy link
Collaborator Author

Yes, let's include .name.

ok

As a principle, I think that we should aim for GeoJSON/JSON to be round-trippable

Pedantically noting that will be impossible for json->geojson->json. Start with JSON without names in refs, process it, it will now have names in refs. (I think?)

I suspect that actually a better principle is to make the best data we can. If there was an error in the original data, should we try and preserve it when roundtripping? I'm not sure why that would be desirable

Maybe this can be a 🥫 of 🪱 🪱 🪱 discussion for later - I'll bear it in mind tho if anything else comes up where this might be related.

@duncandewhurst
Copy link
Contributor

Good points :-)

Pedantically noting that will be impossible for json->geojson->json. Start with JSON without names in refs, process it, it will now have names in refs. (I think?)

Yes, that sounds right.

I suspect that actually a better principle is to make the best data we can. If there was an error in the original data, should we try and preserve it when roundtripping? I'm not sure why that would be desirable

Given that CoVE basically only validates the JSON version of the data (as I understand it), I don't think that we want to explicitly fix errors in either of the conversions to JSON . Doing so would effectively disguise the errors whereas we want the publisher to be aware of them and fix them. I'm less concerned if the conversions from JSON do things like fill in missing names in refs.

Aside from missing names in refs, I haven't thought through what other scenarios this might apply to, but I can think about that more when I come to document a specification for the GeoJSON to JSON conversion.

@odscjames
Copy link
Collaborator Author

I don't think that we want to explicitly fix errors in either of the conversions to JSON . Doing so would effectively disguise the errors

Ah, I meant fix AND have a warning. In the same way the GeoJSON->JSON already gives you a warning for inconsistent references in the meta output eg https://github.com/Open-Telecoms-Data/lib-cove-ofds/blob/v0.4.0/tests/fixtures/geojson_to_json/organisations_inconsistent_1.meta.expected.json#L92

Then in Cove, we can decide how to surface those warnings to the user. I agree we don't want to disguise them.

Aside from missing names in refs, I haven't thought through what other scenarios this might apply to

I haven't either, but it's kinda related to Open-Telecoms-Data/cove-ofds#36

@duncandewhurst
Copy link
Contributor

Sounds good!

odscjames added a commit that referenced this issue Nov 22, 2022
#33

And
* check id is a string
* reorder and comment code to make it easier to read
@odscjames
Copy link
Collaborator Author

Name inclusion now ready for testing on live!

@duncandewhurst
Copy link
Contributor

Looks good!

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

No branches or pull requests

2 participants