Alter the geography columns for local plans and M&W plans - #359
Conversation
|
Thanks for the PR. The goals are clear and the data changes look correct for the current dataset. A few things worth flagging below. |
|
Bug risk: global string replace for separator conversion Both generate-csvs.py and generate_mineral_waste_timetable.py convert the authority-code separator from dash to semicolon using str.replace. This replaces every dash in the entire string, not just the separator between codes. GSS codes like E60000108 do not contain dashes today so it works in practice, but it is a fragile implicit assumption. Splitting on the separator and re-joining would be safer and make the intent explicit. |
|
Fragile authority-column-name inference In generate_mineral_waste_timetable.py the ternary for authorities_column_name silently falls back to waste-planning-authorities for any unexpected geography_column_name value, producing incorrect column names without any error. A guard clause raising ValueError for unexpected values would make failures visible. |
|
Inconsistent null/empty handling - generate-csvs.py returns empty string for missing values while generate_mineral_waste_timetable.py passes through NaN. Downstream consumers may treat these differently; worth picking one convention. Misleading comment - The comment says create authorities column with semicolons instead of dashes if requested. The phrase if requested implies this is conditional, but it always runs unconditionally. PR description vs actual change - The description says the mineral-/waste-planning-authority column is renamed to the plural form, but what actually happens is the original column is renamed to mineral-planning-boundary and a new mineral-planning-authorities column is added alongside it. The description undersells the structural change. No tests for new transformation logic - tests/ has no coverage for the new column derivation logic. A unit test for the separator conversion and an integration assertion that the new columns appear and are correctly formatted in output CSVs would catch regressions. Overall the intent is right and the data looks correct for the existing GSS codes. The main things worth addressing are the global-replace fragility and the missing guard on the column-name inference. |
What type of PR is this? (check all applicable)
Description
This PR:
Related Tickets & Documents