Skip to content

Conversation

@jbousquin
Copy link
Collaborator

Not really ready to merge but I'm interested in the team's feedback. This would be a significant refactor bringing all the get functions into one that kind of dispatches to our general functions using an arg to id the dataset to retrieve params for those functions from a structured collection. Thoughts on:

  1. This refactor approach generally? One quick added value is centralizing all urls lets us do easy checks they are all up.
  2. Use of a named tuple for the collection? I think I was able to capture all the same info from the module without being too verbose, but wasn't super stoked about how much the hierarchy repeats (e.g., assets/education, etc.).

@jbousquin jbousquin linked an issue Sep 26, 2025 that may be closed by this pull request
@tvlombardi
Copy link
Collaborator

Since I haven't run this yet, I'm still wrapping my head around it. Is this refactor an alternative way to run the demo? I understand it isn't completely fleshed out yet.

@jbousquin
Copy link
Collaborator Author

jbousquin commented Sep 29, 2025

Sorry that was really half-baked PR without enough explanation - moving the line of updated test code from the module out to the corresponding test file now. The idea here is that this new get.run_get() function would work across all the retrieval (e.g., get.run_get("library", aoi_gdf) instead of cultural.get_library(aoi_gdf)). That should simplify the package structure and how a user uses it but the tradeoff is this stored data structure (centralizes the information but has to be versatile, not too verbose and maintainable).

Any ideas on a better name for the module/function?

@jbousquin
Copy link
Collaborator Author

Relevant tests (note new code isn't implemented throughout, working in some not in others)
CHAPPIE/tests/test_cultural_assets.py::test_get_historic FAILED
CHAPPIE/tests/test_cultural_assets.py::test_get_library FAILED
CHAPPIE/tests/test_cultural_assets.py::test_get_museums PASSED
CHAPPIE/tests/test_cultural_assets.py::test_get_worship PASSED
CHAPPIE/tests/test_cultural_assets.py::test_get_recAreas PASSED

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

Successfully merging this pull request may close these issues.

Refactor get functions

3 participants