votinginfoproject / vip-specification

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

Fixes the rest.py script to properly order pages in the single-page doc #436

Closed jswiesner closed 2 years ago

jswiesner commented 2 years ago

The current single page doc (e.g. the XML page) is out of order, and the order is not deterministic. This change ensures pages are ordered before they are added to the single-page doc.

This also updates the update_rest_files method to combine the outer loops over mode (xml, csv). I believe the previous implementation could yield unexpected results since type_names and type_map were assembled in the first loop and used in the second.

afsmythe commented 2 years ago

Thanks for catching this! Rendered documentation looks good locally, but I needed to bring in bdf68e7d23c324e3f670f05815ee5b3ca69dbfa7 in order to successfully run python scripts/vip.py make_rest

We'll want to get that commit to vip52 and master.

jswiesner commented 2 years ago

Sounds good, I cherry-picked bdf68e7d23c324e3f670f05815ee5b3ca69dbfa7 and included it in this PR.

jswiesner commented 2 years ago

While testing regenerating the docs using these changes to the Python script, I noticed the table files weren't being generated right (it was generating these files under the case of mode = "csv", whereas the current table files in the repo were generated with mode = "xml"). I'm not sure how it worked in the past since we always process modes in the order of ("xml", "csv"), but I updated the call to update_table_file so we only do this step once using the XML mode (addressed with 0d76bafcb72f40296094d1efa9c5d246d9082533).

afsmythe commented 2 years ago

Sorry, is there any additional information you could add about the "table files"? I'm not seeing any suspicious formatting on rendered documentation built locally.

jswiesner commented 2 years ago

By "table files" I meant the generated files under built_rst/tables. We generate these files with calls to update_table_file, which we currently do twice - once for mode = "xml" and again for mode = "csv". If you comment out the if statement I added around the call to update_table_file and then regenerate the RST files, you'll notice that the files in built_rst/tables are updated to use the csv field names and prefix. I assumed the desired behavior here was to preserve the XML field names, hence I added the if statement to be compatible with the existing files there.

But now that I look at this more closely, I can't find any pages that references the files in built_rst/tables. Are you aware of anything that uses these? If not, we could just delete these files and the code that generates them.

jswiesner commented 2 years ago

I also found this, which looks like we explicitly exclude files from the tables directory when generating the docs: https://github.com/votinginfoproject/vip-specification/blob/c694e79d40b6b71ec9ae94c7cda5a916915c75fb/docs/conf.py#L84

afsmythe commented 2 years ago

Ok, interesting.. this may be a vestige of when the documentation only supported the XML format. CSV was added later and I'm wondering if the tables resource was retained when it could have been removed. Will investigate, but we could probably handle this in another PR if you want to go ahead and merge your code related to the simple page ordering.

afsmythe commented 2 years ago

Noting the tables question might be better handled by the forthcoming PR related to https://github.com/votinginfoproject/vip-specification/issues/279

Happy to take that on within that PR.

jswiesner commented 2 years ago

That sounds great! I also filed #438 so we can separately track the clean up of the tables directory if we do end up confirming it's not needed any more.

I'm going to go ahead and merge this. I'll send follow-up PRs to merge this change into the vip52 and release branches as well, and then also regenerate the docs using the correct ordering.