Conversation
dylanlee
left a comment
dylanlee
left a comment
There was a problem hiding this comment.
Good job! Just have a few minor comments/requested changes on top of the discussed test modifications that we talked about on Teams
src/icefabric/schemas/hydrofabric.py
Outdated
| POI_ID = "poi_id" # HF 2.2 only | ||
| VPU_ID = "vpu_id" | ||
| FP_ID = ("fp_id",) | ||
| FP_ID = "fp_id" |
There was a problem hiding this comment.
FP_ID and SITE_NO don't seem to be referenced anywhere in the codebase. Could we get rid of them here? Also could get rid of VPU_ID if we used QueryIdType VPU_ID inside the CLI.
This would make the IdType enum dedicated to just HF 2.2 related fields which might be less confusing
There was a problem hiding this comment.
Removed all items for NHF and unused items for 2.2, leaving only HL_URI and ID which are used to subset 2.2.
There was a problem hiding this comment.
Turns out POI_ID is used in one of the tests, so I left it in.
app/routers/hydrofabric/router.py
Outdated
There was a problem hiding this comment.
replaced old IdType names with the new ones.
| else: | ||
| nonspatial_layers[table_name] = layer_data | ||
| else: | ||
| print(f"Warning: {table_name} layer is empty") |
There was a problem hiding this comment.
The print statements in router.py should be converted to warning or error logs for consistency with the rest of the app/ code
There was a problem hiding this comment.
Changed print statements to logs
… log, updated tests
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
2 participants
[Short description explaining the high-level reason for the pull request]
Additions
Changed geopackage API endpoint to use generic names for the ID type field that cover both 2.2 and NHF (as applicable) without referring to specific names in the hydrofabric tables such as hl_uri or site_no.
Removals
Changes
Testing
Screenshots
Notes
Todos
Checklist
Testing checklist
Target Environment support
Accessibility
Other