votinginfoproject / vip-specification

The Voting Information Project XML specification.
http://vip-specification.readthedocs.io/en/release/
Other
75 stars 30 forks source link

Updates doc yaml files to incorporate new geospatial modeling #419

Closed jswiesner closed 3 years ago

jswiesner commented 3 years ago

Adds doc pages for the new elements and enums introduced with geospatial modeling for VIP6.

I ended up having to make more decisions than I anticipated around CSV modeling of geospatial data. This can be viewed as a proposed solution to the CSV specification, so please feel free to comment and suggest alternatives.

A few noteworthy aspects of the decisions made for CSV modeling:

afsmythe commented 3 years ago

I modeled this is a single scalar field in the CSV specification which allows for multiple values by pipe delimiting. I didn't see this approach used elsewhere in the CSV spec, but seems like a reasonable solution to me.

There's a similar approach for fields like Precinct.ElectoralDistrictIds and Precinct.PollingLocationIds whereby we delimit on a space. Can we use this same format for external_geospatial_feature.spatial_index?

afsmythe commented 3 years ago

Can we make use of the YAML _sub_types section (see Department) for the geospatial elements?

It looks like SpatialBoundary can be a sub-type for Precinct and would have ExternalGeospatialFeature as it's own sub-type. ExternalFile might remain a "top-level" tag?

Thoughts on this?

jswiesner commented 3 years ago

I modeled this is a single scalar field in the CSV specification which allows for multiple values by pipe delimiting. I didn't see this approach used elsewhere in the CSV spec, but seems like a reasonable solution to me.

There's a similar approach for fields like Precinct.ElectoralDistrictIds and Precinct.PollingLocationIds whereby we delimit on a space. Can we use this same format for external_geospatial_feature.spatial_index?

This sounds good to me, though I don't see it anywhere in the documentation. Is there somewhere this is mentioned? If not, I can add similar documentation to those fields as well. I went ahead and updated the documentation here to use space as the delimiter.

Can we make use of the YAML _sub_types section (see Department) for the geospatial elements?

It looks like SpatialBoundary can be a sub-type for Precinct and would have ExternalGeospatialFeature as it's own sub-type. ExternalFile might remain a "top-level" tag?

Thoughts on this?

Ah I didn't see this option. Using subtypes here is the right thing to do, so I went ahead and added. I also added Checksum as a subtype of ExternalFile.

afsmythe commented 3 years ago

Do you also plan to add in the RST files to this PR?

jswiesner commented 3 years ago

Do you also plan to add in the RST files to this PR?

I saw your recent documentation updates for LeaderPersonIds had YAML and RST changes in separate commits, so I assumed the practice was to split these two into separate PRs. But thinking about it more, these should be in the same PR, even if in separate commits - added.

afsmythe commented 3 years ago

Thanks! And sorry for that confusion.

afsmythe commented 3 years ago

Can you remove those RST files belonging to the elements recently added as subtypes?

One of the benefits of using subtypes is that content is appended to the parent type, and I think since the subtypes were added after your initial make_rest call, they still exist in the repo.

jswiesner commented 3 years ago

Can you remove those RST files belonging to the elements recently added as subtypes?

One of the benefits of using subtypes is that content is appended to the parent type, and I think since the subtypes were added after your initial make_rest call, they still exist in the repo.

Ah sorry about that, you are right. I reverted my last commit with the RST to update it, but didn't realize the process of doing so would cause this PR to be (maybe irreversibly) closed. Before I create a new PR, are you able to reopen this by any chance? The reopen option is grayed out for me.

afsmythe commented 3 years ago

No worries!

The reopen option is also greyed out for me. I'm fine with whatever is easiest/you prefer to do.

jswiesner commented 3 years ago

Moved this change to #420 as there appears to be no way to reopen.